[PATCH 0/3 V2] EDAC, AMD64_EDAC: Add ECC error decoding for newer Fam15h models.

[PATCH 0/3 V2] EDAC, AMD64_EDAC: Add ECC error decoding for newer Fam15h models.

[Aravind Gopalakrishnan]

Changes from V1:
	- Splitting up the patch
	- Remove unnecessary helper functions
	- Add family/model to amd64_pvt
	- Cleanup indent issues

Aravind Gopalakrishnan (3):
  EDAC, AMD64_EDAC: Add PCI Device ID Functions 3 and 4 for newer F15h
    models.
  EDAC, AMD64_EDAC: Add relevant condition checks as F15h M30h does not
    support GART or L3.
  EDAC, AMD64_EDAC: Add ECC decoding support for newer F15h models.

 arch/x86/kernel/amd_nb.c  |   14 ++-
 drivers/edac/amd64_edac.c |  276 +++++++++++++++++++++++++++++++++++++++++----
 drivers/edac/amd64_edac.h |   65 ++++++++++-
 include/linux/pci_ids.h   |    2 +
 4 files changed, 330 insertions(+), 27 deletions(-)

-- 
1.7.10.4

[PATCH 1/3 V2] EDAC, AMD64_EDAC: Add PCI Device ID Functions 3 and 4 for newer F15h models.

[Aravind Gopalakrishnan]

Adding PCI_DEVICE_ID_AMD_15H_NB_M30H_F3 and PCI_DEVICE_ID_AMD_15H_NB_M30H_F4
for F15h model 30h. This is required for the file amd_nb.c

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 3bed2e8..d1fe5d0 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -518,6 +518,8 @@
 #define PCI_DEVICE_ID_AMD_11H_NB_MISC	0x1303
 #define PCI_DEVICE_ID_AMD_11H_NB_LINK	0x1304
 #define PCI_DEVICE_ID_AMD_15H_M10H_F3	0x1403
+#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F3 0x141d
+#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F4 0x141e
 #define PCI_DEVICE_ID_AMD_15H_NB_F0	0x1600
 #define PCI_DEVICE_ID_AMD_15H_NB_F1	0x1601
 #define PCI_DEVICE_ID_AMD_15H_NB_F2	0x1602
-- 
1.7.10.4

Re: [PATCH 1/3 V2] EDAC, AMD64_EDAC: Add PCI Device ID Functions 3 and 4 for newer F15h models.

[Borislav Petkov]

On Fri, Aug 02, 2013 at 05:43:02PM -0500, Aravind Gopalakrishnan wrote:
> Adding PCI_DEVICE_ID_AMD_15H_NB_M30H_F3 and PCI_DEVICE_ID_AMD_15H_NB_M30H_F4
> for F15h model 30h. This is required for the file amd_nb.c

... and amd64_edac.c

Also, remember to read the comment at the beginning of
include/linux/pci_ids.h!

I fixed up the commit message when applying.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/3 V2] EDAC, AMD64_EDAC: Add PCI Device ID Functions 3 and 4 for newer F15h models.

[Aravind Gopalakrishnan]

On 8/6/2013 3:17 PM, Borislav Petkov wrote:
> On Fri, Aug 02, 2013 at 05:43:02PM -0500, Aravind Gopalakrishnan wrote:
>> Adding PCI_DEVICE_ID_AMD_15H_NB_M30H_F3 and PCI_DEVICE_ID_AMD_15H_NB_M30H_F4
>> for F15h model 30h. This is required for the file amd_nb.c
> ... and amd64_edac.c
>
> Also, remember to read the comment at the beginning of
> include/linux/pci_ids.h!
>
> I fixed up the commit message when applying.
>
Okay, Thanks!

[PATCH 2/3 V2] EDAC, AMD64_EDAC: Add relevant condition checks as F15h M30h does not support GART or L3.

[Aravind Gopalakrishnan]

Adding code to check for specific model (F15h, M30h) and if yes,
do not add flag AMD_NB_GART. Also check cpuid_edx(0x80000006) for
prescence of L3. If no L3, do not add any L3 flags.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>

diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
index 3048ded..3ee7a4d 100644
--- a/arch/x86/kernel/amd_nb.c
+++ b/arch/x86/kernel/amd_nb.c
@@ -20,6 +20,7 @@ const struct pci_device_id amd_nb_misc_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M10H_F3) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F3) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3) },
 	{}
 };
@@ -27,6 +28,7 @@ EXPORT_SYMBOL(amd_nb_misc_ids);
 
 static const struct pci_device_id amd_nb_link_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
 	{}
 };
@@ -81,13 +83,21 @@ int amd_cache_northbridges(void)
 			next_northbridge(misc, amd_nb_misc_ids);
 		node_to_amd_nb(i)->link = link =
 			next_northbridge(link, amd_nb_link_ids);
-        }
+	}
 
+	/* GART present only on Fam15h upto model 0fh */
 	if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
-	    boot_cpu_data.x86 == 0x15)
+	    (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
 		amd_northbridges.flags |= AMD_NB_GART;
 
 	/*
+	 * Check CPUID Fn8000_0006_EDX: L3 Cache Identifiers.
+	 * If == 0, then no need to proceed as there is no L3.
+	 */
+	if (cpuid_edx(0x80000006) == 0)
+		return 0;
+
+	/*
 	 * Some CPU families support L3 Cache Index Disable. There are some
 	 * limitations because of E382 and E388 on family 0x10.
 	 */
-- 
1.7.10.4

Re: [PATCH 2/3 V2] EDAC, AMD64_EDAC: Add relevant condition checks as F15h M30h does not support GART or L3.

[Borislav Petkov]

On Fri, Aug 02, 2013 at 05:43:03PM -0500, Aravind Gopalakrishnan wrote:
> Adding code to check for specific model (F15h, M30h) and if yes,
> do not add flag AMD_NB_GART. Also check cpuid_edx(0x80000006) for
> prescence of L3. If no L3, do not add any L3 flags.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> 
> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> index 3048ded..3ee7a4d 100644
> --- a/arch/x86/kernel/amd_nb.c
> +++ b/arch/x86/kernel/amd_nb.c
> @@ -20,6 +20,7 @@ const struct pci_device_id amd_nb_misc_ids[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M10H_F3) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F3) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3) },
>  	{}
>  };
> @@ -27,6 +28,7 @@ EXPORT_SYMBOL(amd_nb_misc_ids);
>  
>  static const struct pci_device_id amd_nb_link_ids[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
>  	{}
>  };
> @@ -81,13 +83,21 @@ int amd_cache_northbridges(void)
>  			next_northbridge(misc, amd_nb_misc_ids);
>  		node_to_amd_nb(i)->link = link =
>  			next_northbridge(link, amd_nb_link_ids);
> -        }
> +	}
>  
> +	/* GART present only on Fam15h upto model 0fh */
>  	if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
> -	    boot_cpu_data.x86 == 0x15)
> +	    (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
>  		amd_northbridges.flags |= AMD_NB_GART;
>  
>  	/*
> +	 * Check CPUID Fn8000_0006_EDX: L3 Cache Identifiers.
> +	 * If == 0, then no need to proceed as there is no L3.

This comment explains the code. Just sit down and think about it: does
it make any sense to have that in a comment?

> +	 */
> +	if (cpuid_edx(0x80000006) == 0)
> +		return 0;
> +
> +	/*
>  	 * Some CPU families support L3 Cache Index Disable. There are some
>  	 * limitations because of E382 and E388 on family 0x10.
>  	 */
> --

I fixed it up like this:

commit a0cb1bab68823648def0fda19bf307ad08eb25d2
Author: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Date:   Fri Aug 2 17:43:03 2013 -0500

    x86, amd_nb: Clarify F15h, model 30h GART and L3 support
    
    F15h, models 0x30 and later don't have a GART. Note that. Also check
    cpuid_edx(0x80000006) for prescence of L3 because there are models which
    don't sport an L3.
    
    Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
    [ Boris: rewrite commit message, cleanup comments. ]
    Signed-off-by: Borislav Petkov <bp@suse.de>

diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
index 3048ded1b598..59554dca96ec 100644
--- a/arch/x86/kernel/amd_nb.c
+++ b/arch/x86/kernel/amd_nb.c
@@ -20,6 +20,7 @@ const struct pci_device_id amd_nb_misc_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M10H_F3) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F3) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3) },
 	{}
 };
@@ -27,6 +28,7 @@ EXPORT_SYMBOL(amd_nb_misc_ids);
 
 static const struct pci_device_id amd_nb_link_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
 	{}
 };
@@ -81,13 +83,20 @@ int amd_cache_northbridges(void)
 			next_northbridge(misc, amd_nb_misc_ids);
 		node_to_amd_nb(i)->link = link =
 			next_northbridge(link, amd_nb_link_ids);
-        }
+	}
 
+	/* GART present only on Fam15h upto model 0fh */
 	if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
-	    boot_cpu_data.x86 == 0x15)
+	    (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
 		amd_northbridges.flags |= AMD_NB_GART;
 
 	/*
+	 * Check for L3 cache presence.
+	 */
+	if (!cpuid_edx(0x80000006))
+		return 0;
+
+	/*
 	 * Some CPU families support L3 Cache Index Disable. There are some
 	 * limitations because of E382 and E388 on family 0x10.
 	 */



-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 2/3 V2] EDAC, AMD64_EDAC: Add relevant condition checks as F15h M30h does not support GART or L3.

[Aravind Gopalakrishnan]

On 8/6/2013 3:19 PM, Borislav Petkov wrote:
> On Fri, Aug 02, 2013 at 05:43:03PM -0500, Aravind Gopalakrishnan wrote:
>> Adding code to check for specific model (F15h, M30h) and if yes,
>> do not add flag AMD_NB_GART. Also check cpuid_edx(0x80000006) for
>> prescence of L3. If no L3, do not add any L3 flags.
>>
>> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>
>> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
>> index 3048ded..3ee7a4d 100644
>> --- a/arch/x86/kernel/amd_nb.c
>> +++ b/arch/x86/kernel/amd_nb.c
>> @@ -20,6 +20,7 @@ const struct pci_device_id amd_nb_misc_ids[] = {
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3) },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M10H_F3) },
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F3) },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3) },
>>   	{}
>>   };
>> @@ -27,6 +28,7 @@ EXPORT_SYMBOL(amd_nb_misc_ids);
>>   
>>   static const struct pci_device_id amd_nb_link_ids[] = {
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
>>   	{}
>>   };
>> @@ -81,13 +83,21 @@ int amd_cache_northbridges(void)
>>   			next_northbridge(misc, amd_nb_misc_ids);
>>   		node_to_amd_nb(i)->link = link =
>>   			next_northbridge(link, amd_nb_link_ids);
>> -        }
>> +	}
>>   
>> +	/* GART present only on Fam15h upto model 0fh */
>>   	if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
>> -	    boot_cpu_data.x86 == 0x15)
>> +	    (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
>>   		amd_northbridges.flags |= AMD_NB_GART;
>>   
>>   	/*
>> +	 * Check CPUID Fn8000_0006_EDX: L3 Cache Identifiers.
>> +	 * If == 0, then no need to proceed as there is no L3.
> This comment explains the code. Just sit down and think about it: does
> it make any sense to have that in a comment?
>> +	 */
>> +	if (cpuid_edx(0x80000006) == 0)
>> +		return 0;
>> +
>> +	/*
>>   	 * Some CPU families support L3 Cache Index Disable. There are some
>>   	 * limitations because of E382 and E388 on family 0x10.
>>   	 */
>> --
> I fixed it up like this:
>
> commit a0cb1bab68823648def0fda19bf307ad08eb25d2
> Author: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> Date:   Fri Aug 2 17:43:03 2013 -0500
>
>      x86, amd_nb: Clarify F15h, model 30h GART and L3 support
>      
>      F15h, models 0x30 and later don't have a GART. Note that. Also check
>      cpuid_edx(0x80000006) for prescence of L3 because there are models which
>      don't sport an L3.
>      
>      Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>      [ Boris: rewrite commit message, cleanup comments. ]
>      Signed-off-by: Borislav Petkov <bp@suse.de>

Okay, Thanks!
> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> index 3048ded1b598..59554dca96ec 100644
> --- a/arch/x86/kernel/amd_nb.c
> +++ b/arch/x86/kernel/amd_nb.c
> @@ -20,6 +20,7 @@ const struct pci_device_id amd_nb_misc_ids[] = {
>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3) },
>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M10H_F3) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F3) },
>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3) },
>   	{}
>   };
> @@ -27,6 +28,7 @@ EXPORT_SYMBOL(amd_nb_misc_ids);
>   
>   static const struct pci_device_id amd_nb_link_ids[] = {
>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
>   	{}
>   };
> @@ -81,13 +83,20 @@ int amd_cache_northbridges(void)
>   			next_northbridge(misc, amd_nb_misc_ids);
>   		node_to_amd_nb(i)->link = link =
>   			next_northbridge(link, amd_nb_link_ids);
> -        }
> +	}
>   
> +	/* GART present only on Fam15h upto model 0fh */
>   	if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
> -	    boot_cpu_data.x86 == 0x15)
> +	    (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
>   		amd_northbridges.flags |= AMD_NB_GART;
>   
>   	/*
> +	 * Check for L3 cache presence.
> +	 */
> +	if (!cpuid_edx(0x80000006))
> +		return 0;
> +
> +	/*
>   	 * Some CPU families support L3 Cache Index Disable. There are some
>   	 * limitations because of E382 and E388 on family 0x10.
>   	 */
>
>
>

[PATCH 3/3 V2] EDAC, AMD64_EDAC: Add ECC decoding support for newer F15h models.

[Aravind Gopalakrishnan]

Adding support for handling ECC error decoding for new F15 models.
On newer models, support has been included for upto 4 DCT's,
however, only DCT0 and DCT3 are currently configured. (Refer BKDG Section 2.10)
There is also a new "Routing DRAM Requests" algorithm for this model.

Tested on Fam15h M30h with ECC turned on using mce_amd_inj facility and
verified to be functionally correct.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 8b6a034..42dab12 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -123,7 +123,11 @@ static void f15h_select_dct(struct amd64_pvt *pvt, u8 dct)
 	u32 reg = 0;
 
 	amd64_read_pci_cfg(pvt->F1, DCT_CFG_SEL, &reg);
-	reg &= 0xfffffffe;
+	/*
+	 * If Fam15h M30h, mask out last two bits for programming dct.
+	 * Else, only mask out the last bit.
+	 */
+	reg &= (F15_M30H_CPUS) ? 0xfffffffc : 0xfffffff8;
 	reg |= dct;
 	amd64_write_pci_cfg(pvt->F1, DCT_CFG_SEL, reg);
 }
@@ -133,8 +137,12 @@ static int f15_read_dct_pci_cfg(struct amd64_pvt *pvt, int addr, u32 *val,
 {
 	u8 dct  = 0;
 
+	/*
+	 * For F15 M30h, the second dct is DCT 3.
+	 * Refer BKDG Section 2.10
+	 */
 	if (addr >= 0x140 && addr <= 0x1a0) {
-		dct   = 1;
+		dct = F15H_M30H_CPU ? 3 : 1;
 		addr -= 0x100;
 	}
 
@@ -205,8 +213,9 @@ static int amd64_set_scrub_rate(struct mem_ctl_info *mci, u32 bw)
 	if (boot_cpu_data.x86 == 0xf)
 		min_scrubrate = 0x0;
 
-	/* F15h Erratum #505 */
-	if (boot_cpu_data.x86 == 0x15)
+	/* F15h Models 0x00 - 0x0f Erratum #505 */
+	if (boot_cpu_data.x86 == 0x15 &&
+		boot_cpu_data.x86_model != 0x30)
 		f15h_select_dct(pvt, 0);
 
 	return __amd64_set_scrub_rate(pvt->F3, bw, min_scrubrate);
@@ -218,8 +227,9 @@ static int amd64_get_scrub_rate(struct mem_ctl_info *mci)
 	u32 scrubval = 0;
 	int i, retval = -EINVAL;
 
-	/* F15h Erratum #505 */
-	if (boot_cpu_data.x86 == 0x15)
+	/* F15h Models 0x00 - 0x0f Erratum #505 */
+	if (boot_cpu_data.x86 == 0x15 &&
+		boot_cpu_data.x86_model != 0x30)
 		f15h_select_dct(pvt, 0);
 
 	amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
@@ -343,10 +353,10 @@ static void get_cs_base_and_mask(struct amd64_pvt *pvt, int csrow, u8 dct,
 		addr_shift	= 4;
 
 	/*
-	* F16h needs two addr_shift values: 8 for high and 6 for low
-	* (cf. F16h BKDG).
+	* F16h and F15h(model 30h) needs two addr_shift values:
+	* 8 for high and 6 for low (cf. F16h BKDG).
 	*/
-	} else if (boot_cpu_data.x86 == 0x16) {
+	} else if (boot_cpu_data.x86 == 0x16 || F15H_M30H_CPU) {
 		csbase          = pvt->csels[dct].csbases[csrow];
 		csmask          = pvt->csels[dct].csmasks[csrow >> 1];
 
@@ -743,6 +753,9 @@ static void prep_chip_selects(struct amd64_pvt *pvt)
 	if (boot_cpu_data.x86 == 0xf && pvt->ext_model < K8_REV_F) {
 		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
 		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 8;
+	} else if (F15H_M30H_CPU) {
+		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
+		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
 	} else {
 		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
 		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
@@ -850,9 +863,9 @@ static u64 get_error_address(struct mce *m)
 	addr = m->addr & GENMASK(start_bit, end_bit);
 
 	/*
-	 * Erratum 637 workaround
+	 * Erratum 637 workaround for F15h Models 0x00 - 0x0f
 	 */
-	if (c->x86 == 0x15) {
+	if (c->x86 == 0x15 && c->x86_model != 0x30) {
 		struct amd64_pvt *pvt;
 		u64 cc6_base, tmp_addr;
 		u32 tmp;
@@ -918,6 +931,7 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
 	struct amd_northbridge *nb;
 	struct pci_dev *misc, *f1 = NULL;
 	struct cpuinfo_x86 *c = &boot_cpu_data;
+	unsigned int pci_func;
 	int off = range << 3;
 	u32 llim;
 
@@ -942,7 +956,12 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
 		return;
 
 	misc = nb->misc;
-	f1 = pci_get_related_function(misc->vendor, PCI_DEVICE_ID_AMD_15H_NB_F1, misc);
+
+	pci_func = (c->x86_model == 0x30) ? PCI_DEVICE_ID_AMD_15H_M30H_NB_F1
+					  : PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
+
+	f1 = pci_get_related_function(misc->vendor, pci_func, misc);
+
 	if (WARN_ON(!f1))
 		return;
 
@@ -1173,7 +1192,8 @@ static int f15_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
 }
 
 /*
- * F16h has only limited cs_modes
+ * F16h has only limited cs_modes.
+ * F15 M30h follows the same pattern.
  */
 static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
 				unsigned cs_mode)
@@ -1218,6 +1238,51 @@ static void read_dram_ctl_register(struct amd64_pvt *pvt)
 }
 
 /*
+ * Determine channel (DCT) based on the interleaving mode:
+ * F15h M30h BKDG, 2.10.12 Memory Interleaving Modes.
+ */
+static u8 f15_m30h_determine_channel(struct amd64_pvt *pvt, u64 sys_addr,
+				u8 intlv_en, int num_dcts_intlv, u32 dct_sel)
+{
+	u8 channel = 0;
+	u8 select;
+	u8 intlv_addr = dct_sel_interleave_addr(pvt);
+
+	if (!(intlv_en))
+		return (u8)(dct_sel);
+	/*
+	* If num_dcts_intlv == 2, them consider addr bit 8 or addr bit 9
+	* depending on intlv_addr, then return either channel = 0/1/2/3.
+	* If num_dcts_intlv == 4, consider either bits[8:9] or bits[9:10]
+	* depending on intlv_addr, then return the value obtained.
+	*/
+
+	if (num_dcts_intlv == 2) {
+		if (intlv_addr == 0x4)
+			select = (sys_addr >> 8) & BIT(0);
+		else if (intlv_addr == 0x5)
+			select = (sys_addr >> 9) & BIT(0);
+		else
+			return -EINVAL;
+
+		/* If select !=0, upper DCT selected, else lower DCT */
+		channel = select ? 0x3 : 0;
+
+	} else if (num_dcts_intlv == 4) {
+		if (intlv_addr == 0x4)
+			select =  (sys_addr >> 8) & 0x3;
+		else if (intlv_addr == 0x5)
+			select = (sys_addr >> 9) & 0x3;
+		else
+			return -EINVAL;
+
+		channel = select;
+	}
+
+	return channel;
+}
+
+/*
  * Determine channel (DCT) based on the interleaving mode: F10h BKDG, 2.8.9 Memory
  * Interleaving Modes.
  */
@@ -1366,6 +1431,10 @@ static int f1x_lookup_addr_in_dct(u64 in_addr, u8 nid, u8 dct)
 			 (in_addr & cs_mask), (cs_base & cs_mask));
 
 		if ((in_addr & cs_mask) == (cs_base & cs_mask)) {
+			if (F15H_M30H_CPU) {
+				cs_found =  csrow;
+				break;
+			}
 			cs_found = f10_process_possible_spare(pvt, dct, csrow);
 
 			edac_dbg(1, " MATCH csrow=%d\n", cs_found);
@@ -1492,20 +1561,151 @@ static int f1x_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
 	return cs_found;
 }
 
-static int f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt, u64 sys_addr,
-				       int *chan_sel)
+/*
+ * Routing DRAM Requests algorithm is different for F15h M30h.
+ * It is cleaner to use a brand new function rather than
+ * adding quirks in the more generic 'f1x_match_to_this_node'.
+ * Refer "2.10.5 DRAM Routing Requests" in BKDG for info.
+ */
+static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
+				  u64 sys_addr, int *chan_sel)
+{
+	int cs_found = -EINVAL;
+	int num_dcts_intlv = 0;
+	u64 chan_addr, chan_offset, high_addr_offset;
+	u64 dct_base, dct_limit;
+	u32 dct_cont_base_reg, dct_cont_limit_reg, tmp;
+	u8 channel, alias_channel, leg_mmio_hole;
+
+
+	/*  Read DRAM Control registers specific to F15 M30h. */
+	amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &dct_cont_base_reg);
+	amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &dct_cont_limit_reg);
+
+	/* Extract F15h M30h specific config */
+	u64 dhar_offset		= f10_dhar_offset(pvt);
+	u8 dct_offset_en	= (u8) ((dct_cont_base_reg >> 3) & BIT(0));
+	u8 dct_sel		= (u8) ((dct_cont_base_reg >> 4) & 0x7);
+	u8 intlv_addr		= dct_sel_interleave_addr(pvt);
+	u8 node_id		= dram_dst_node(pvt, range);
+	u8 intlv_en		= dram_intlv_en(pvt, range);
+
+	edac_dbg(1, "(range %d) SystemAddr= 0x%llx Limit=0x%llx\n",
+		 range, sys_addr, get_dram_limit(pvt, range));
+
+	if (!(get_dram_base(pvt, range)  <= sys_addr) &&
+	    !(get_dram_limit(pvt, range) >= sys_addr))
+		return -EINVAL;
+
+	if (dhar_valid(pvt) &&
+	    dhar_base(pvt) <= sys_addr &&
+	    sys_addr < BIT_64(32)) {
+		amd64_warn("Huh? Address is in the MMIO hole: 0x%016llx\n",
+			    sys_addr);
+		return -EINVAL;
+	}
+
+	/* Verify sys_addr is within DCT Range. */
+	dct_base = (dct_sel_baseaddr(pvt) << 27);
+	dct_limit = (((dct_cont_limit_reg >> 11) & 0x1FFF) << 27) | 0x7FFFFFF;
+	if (!(dct_cont_base_reg & BIT(0)) &&
+	    !(dct_base <= sys_addr && dct_limit >= sys_addr)) {
+		return -EINVAL;
+	}
+
+	/* Verify number of dct's that participate in channel interleaving. */
+	num_dcts_intlv = (int) hweight8(intlv_en);
+
+	if (!(num_dcts_intlv % 2 == 0) || (num_dcts_intlv > 4))
+		return -EINVAL;
+
+	channel = f15_m30h_determine_channel(pvt, sys_addr, intlv_en,
+					     num_dcts_intlv, dct_sel);
+
+	/* Verify if we stay within the MAX number of channels allowed */
+	if (channel > 4 || channel < 0)
+		return -EINVAL;
+
+	leg_mmio_hole = (u8) (dct_cont_base_reg >> 1 & BIT(0));
+
+	/* Get normalized DCT addr */
+	if (leg_mmio_hole && (sys_addr >= BIT_64(32)))
+		chan_offset = dhar_offset;
+	else
+		chan_offset = dct_base;
+
+	chan_addr = sys_addr - chan_offset;
+
+	/* remove channel interleave */
+	if (num_dcts_intlv == 2) {
+		if (intlv_addr == 0x4)
+			chan_addr = ((chan_addr >> 9) << 8) |
+						(chan_addr & 0xff);
+		else if (intlv_addr == 0x5)
+			chan_addr = ((chan_addr >> 10) << 9) |
+						(chan_addr & 0x1ff);
+		else
+			return -EINVAL;
+
+	} else if (num_dcts_intlv == 4) {
+		if (intlv_addr == 0x4)
+			chan_addr = ((chan_addr >> 10) << 8) |
+							(chan_addr & 0xff);
+		else if (intlv_addr == 0x5)
+			chan_addr = ((chan_addr >> 11) << 9) |
+							(chan_addr & 0x1ff);
+		else
+			return -EINVAL;
+	}
+
+	if (dct_offset_en) {
+		amd64_read_pci_cfg(pvt->F1,
+				   DRAM_CONT_HIGH_OFF + (int) channel * 4,
+				   &tmp);
+		chan_addr +=  ((tmp >> 11) & 0xfff) << 27;
+	}
+
+	/* Set DCTCFGSEL = ChannelSelect */
+	f15h_select_dct(pvt, channel);
+
+	edac_dbg(1, "   Normalized DCT addr: 0x%llx\n", chan_addr);
+	/* Find the Chip select.. */
+	/*
+	 * NOTE: if channel = 3, then alias it to 1.
+	 * This is because, in F15 M30h, there is support for
+	 * 4 DCT's, but only 2 are currently included in the architecture.
+	 * They are DCT0 and DCT3. But we have read all registers of DCT3
+	 * into pvt->csels[1]. So we need to use '1' here to get correct
+	 * info. Refer F15 M30h BKDG Section 2.10 and 2.10.3 for clarifications
+	 */
+
+	alias_channel =  (channel == 3) ? 1 : channel;
+
+	cs_found = f1x_lookup_addr_in_dct(chan_addr, node_id, alias_channel);
+
+	if (cs_found >= 0)
+		*chan_sel = alias_channel;
+
+	return cs_found;
+}
+
+static int
+f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt,
+			    u64 sys_addr,
+			    int *chan_sel)
 {
 	int cs_found = -EINVAL;
 	unsigned range;
 
 	for (range = 0; range < DRAM_RANGES; range++) {
-
 		if (!dram_rw(pvt, range))
 			continue;
-
-		if ((get_dram_base(pvt, range)  <= sys_addr) &&
-		    (get_dram_limit(pvt, range) >= sys_addr)) {
-
+		if (F15H_M30H_CPU)
+			cs_found = f15_m30h_match_to_this_node(pvt, range,
+							       sys_addr,
+							       chan_sel);
+		else if ((get_dram_base(pvt, range)  <= sys_addr) &&
+			 (get_dram_limit(pvt, range) >= sys_addr)) {
 			cs_found = f1x_match_to_this_node(pvt, range,
 							  sys_addr, chan_sel);
 			if (cs_found >= 0)
@@ -1624,6 +1824,17 @@ static struct amd64_family_type amd64_family_types[] = {
 			.read_dct_pci_cfg	= f15_read_dct_pci_cfg,
 		}
 	},
+	[F15_M30H_CPUS] = {
+		.ctl_name = "F15h_M30h",
+		.f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1,
+		.f3_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F3,
+		.ops = {
+			.early_channel_count	= f1x_early_channel_count,
+			.map_sysaddr_to_csrow	= f1x_map_sysaddr_to_csrow,
+			.dbam_to_cs		= f16_dbam_to_chip_select,
+			.read_dct_pci_cfg	= f15_read_dct_pci_cfg,
+		}
+	},
 	[F16_CPUS] = {
 		.ctl_name = "F16h",
 		.f1_id = PCI_DEVICE_ID_AMD_16H_NB_F1,
@@ -2388,27 +2599,44 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci,
 static struct amd64_family_type *amd64_per_family_init(struct amd64_pvt *pvt)
 {
 	u8 fam = boot_cpu_data.x86;
+	u8 model = boot_cpu_data.x86_model;
 	struct amd64_family_type *fam_type = NULL;
 
 	switch (fam) {
 	case 0xf:
 		fam_type		= &amd64_family_types[K8_CPUS];
 		pvt->ops		= &amd64_family_types[K8_CPUS].ops;
+		pvt->family		= fam;
+		pvt->model		= model;
 		break;
 
 	case 0x10:
 		fam_type		= &amd64_family_types[F10_CPUS];
 		pvt->ops		= &amd64_family_types[F10_CPUS].ops;
+		pvt->family		= fam;
+		pvt->model		= model;
 		break;
 
 	case 0x15:
+		if (model == 0x30) {
+			fam_type	= &amd64_family_types[F15_M30H_CPUS];
+			pvt->ops	= &amd64_family_types[F15_M30H_CPUS].ops;
+			pvt->family	= fam;
+			pvt->model	= model;
+			break;
+		}
+
 		fam_type		= &amd64_family_types[F15_CPUS];
 		pvt->ops		= &amd64_family_types[F15_CPUS].ops;
+		pvt->family		= fam;
+		pvt->model		= model;
 		break;
 
 	case 0x16:
 		fam_type		= &amd64_family_types[F16_CPUS];
 		pvt->ops		= &amd64_family_types[F16_CPUS].ops;
+		pvt->family		= fam;
+		pvt->model		= model;
 		break;
 
 	default:
@@ -2638,6 +2866,14 @@ static DEFINE_PCI_DEVICE_TABLE(amd64_pci_table) = {
 	},
 	{
 		.vendor		= PCI_VENDOR_ID_AMD,
+		.device		= PCI_DEVICE_ID_AMD_15H_M30H_NB_F2,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.class		= 0,
+		.class_mask	= 0,
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_AMD,
 		.device		= PCI_DEVICE_ID_AMD_16H_NB_F2,
 		.subvendor	= PCI_ANY_ID,
 		.subdevice	= PCI_ANY_ID,
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 2c6f113..ff15f14 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -170,6 +170,8 @@
 /*
  * PCI-defined configuration space registers
  */
+#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F1 0x141b
+#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F2 0x141c
 #define PCI_DEVICE_ID_AMD_15H_NB_F1	0x1601
 #define PCI_DEVICE_ID_AMD_15H_NB_F2	0x1602
 #define PCI_DEVICE_ID_AMD_16H_NB_F1	0x1531
@@ -181,13 +183,24 @@
 #define DRAM_BASE_LO			0x40
 #define DRAM_LIMIT_LO			0x44
 
-#define dram_intlv_en(pvt, i)		((u8)((pvt->ranges[i].base.lo >> 8) & 0x7))
+/*
+ * F15 M30h DRAM Controller Base/Limit
+ * D18F1x2[1C:00]
+ */
+#define DRAM_CONT_BASE			0x200
+#define DRAM_CONT_LIMIT			0x204
+
+/*
+ * F15 M30h DRAM Controller High Addre Offset
+ * D18F1x2[4C:40]
+ */
+#define DRAM_CONT_HIGH_OFF      0x240
+
 #define dram_rw(pvt, i)			((u8)(pvt->ranges[i].base.lo & 0x3))
 #define dram_intlv_sel(pvt, i)		((u8)((pvt->ranges[i].lim.lo >> 8) & 0x7))
 #define dram_dst_node(pvt, i)		((u8)(pvt->ranges[i].lim.lo & 0x7))
 
 #define DHAR				0xf0
-#define dhar_valid(pvt)			((pvt)->dhar & BIT(0))
 #define dhar_mem_hoist_valid(pvt)	((pvt)->dhar & BIT(1))
 #define dhar_base(pvt)			((pvt)->dhar & 0xff000000)
 #define k8_dhar_offset(pvt)		(((pvt)->dhar & 0x0000ff00) << 16)
@@ -234,8 +247,6 @@
 #define DDR3_MODE			BIT(8)
 
 #define DCT_SEL_LO			0x110
-#define dct_sel_baseaddr(pvt)		((pvt)->dct_sel_lo & 0xFFFFF800)
-#define dct_sel_interleave_addr(pvt)	(((pvt)->dct_sel_lo >> 6) & 0x3)
 #define dct_high_range_enabled(pvt)	((pvt)->dct_sel_lo & BIT(0))
 #define dct_interleave_enabled(pvt)	((pvt)->dct_sel_lo & BIT(2))
 
@@ -293,10 +304,14 @@
 /* MSRs */
 #define MSR_MCGCTL_NBE			BIT(4)
 
+/* Helper Macro for F15h M30h */
+#define F15H_M30H_CPU			((pvt->family == 0x15) && \
+					 (pvt->model == 0x30))
 enum amd_families {
 	K8_CPUS = 0,
 	F10_CPUS,
 	F15_CPUS,
+	F15_M30H_CPUS,
 	F16_CPUS,
 	NUM_FAMILIES,
 };
@@ -337,6 +352,8 @@ struct amd64_pvt {
 	struct pci_dev *F1, *F2, *F3;
 
 	u16 mc_node_id;		/* MC index of this MC node */
+	u8 family;		/* CPU family */
+	u8 model;		/* CPU model */
 	int ext_model;		/* extended model value of this node */
 	int channel_count;
 
@@ -414,6 +431,14 @@ static inline u16 extract_syndrome(u64 status)
 	return ((status >> 47) & 0xff) | ((status >> 16) & 0xff00);
 }
 
+static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
+{
+	if (F15H_M30H_CPU)
+		return (((pvt->dct_sel_hi >> 9) & 0x1) << 2) |
+			((pvt->dct_sel_lo >> 6) & 0x3);
+
+	return	((pvt)->dct_sel_lo >> 6) & 0x3;
+}
 /*
  * per-node ECC settings descriptor
  */
@@ -504,3 +529,33 @@ static inline void enable_caches(void *dummy)
 {
 	write_cr0(read_cr0() & ~X86_CR0_CD);
 }
+
+static inline u8 dram_intlv_en(struct amd64_pvt *pvt, unsigned int i)
+{
+	u32 tmp;
+	if (F15H_M30H_CPU) {
+		amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &tmp);
+		return (u8) tmp & 0xF;
+	}
+	return (u8) (pvt->ranges[i].base.lo >> 8) & 0x7;
+}
+
+static inline u8 dhar_valid(struct amd64_pvt *pvt)
+{
+	u32 tmp;
+	if (F15H_M30H_CPU) {
+		amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
+		return (tmp >> 1) & BIT(0);
+	}
+	return (pvt)->dhar & BIT(0);
+}
+
+static inline u32 dct_sel_baseaddr(struct amd64_pvt *pvt)
+{
+	u32 tmp;
+	if (F15H_M30H_CPU) {
+		amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
+		return (tmp >> 11) & 0x1FFF;
+	}
+	return (pvt)->dct_sel_lo & 0xFFFFF800;
+}
-- 
1.7.10.4

Re: [PATCH 3/3 V2] EDAC, AMD64_EDAC: Add ECC decoding support for newer F15h models.

[Borislav Petkov]

On Fri, Aug 02, 2013 at 05:43:04PM -0500, Aravind Gopalakrishnan wrote:
> Adding support for handling ECC error decoding for new F15 models.
> On newer models, support has been included for upto 4 DCT's,
> however, only DCT0 and DCT3 are currently configured. (Refer BKDG Section 2.10)
> There is also a new "Routing DRAM Requests" algorithm for this model.
> 
> Tested on Fam15h M30h with ECC turned on using mce_amd_inj facility and
> verified to be functionally correct.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 8b6a034..42dab12 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -123,7 +123,11 @@ static void f15h_select_dct(struct amd64_pvt *pvt, u8 dct)
>  	u32 reg = 0;
>  
>  	amd64_read_pci_cfg(pvt->F1, DCT_CFG_SEL, &reg);
> -	reg &= 0xfffffffe;
> +	/*
> +	 * If Fam15h M30h, mask out last two bits for programming dct.
> +	 * Else, only mask out the last bit.
> +	 */

No need for that comment.

> +	reg &= (F15_M30H_CPUS) ? 0xfffffffc : 0xfffffff8;

Let's see, I had 0xfffffffe and now you have 0xfffffff8. See the
difference?

Also, F15H_M30H_CPUS is an enum, AFAICT, and it will always be true as
long as it is != 0. What you meant here is "F15H_M30H_CPU". So you see
how those defines are misleading and made you use them wrong.

So let's correct it all and make it more clear:

	reg &= (pvt->model >= 0x30) ? ~3 : ~1;

we don't need to look at the family because this function -
f15h_select_dct - is called on F15h only anyway.

>  	reg |= dct;
>  	amd64_write_pci_cfg(pvt->F1, DCT_CFG_SEL, reg);
>  }
> @@ -133,8 +137,12 @@ static int f15_read_dct_pci_cfg(struct amd64_pvt *pvt, int addr, u32 *val,
>  {
>  	u8 dct  = 0;
>  
> +	/*
> +	 * For F15 M30h, the second dct is DCT 3.
> +	 * Refer BKDG Section 2.10
> +	 */
>  	if (addr >= 0x140 && addr <= 0x1a0) {
> -		dct   = 1;
> +		dct = F15H_M30H_CPU ? 3 : 1;

ditto here: use pvt->model like above.

>  		addr -= 0x100;
>  	}
>  
> @@ -205,8 +213,9 @@ static int amd64_set_scrub_rate(struct mem_ctl_info *mci, u32 bw)
>  	if (boot_cpu_data.x86 == 0xf)
>  		min_scrubrate = 0x0;
>  
> -	/* F15h Erratum #505 */
> -	if (boot_cpu_data.x86 == 0x15)
> +	/* F15h Models 0x00 - 0x0f Erratum #505 */

Hmm, are you sure this erratum is fixed in SR? Talk to Eric about it
first, please. RG says "No fix planned".

> +	if (boot_cpu_data.x86 == 0x15 &&
> +		boot_cpu_data.x86_model != 0x30)
>  		f15h_select_dct(pvt, 0);
>  
>  	return __amd64_set_scrub_rate(pvt->F3, bw, min_scrubrate);
> @@ -218,8 +227,9 @@ static int amd64_get_scrub_rate(struct mem_ctl_info *mci)
>  	u32 scrubval = 0;
>  	int i, retval = -EINVAL;
>  
> -	/* F15h Erratum #505 */
> -	if (boot_cpu_data.x86 == 0x15)
> +	/* F15h Models 0x00 - 0x0f Erratum #505 */

ditto.

> +	if (boot_cpu_data.x86 == 0x15 &&
> +		boot_cpu_data.x86_model != 0x30)
>  		f15h_select_dct(pvt, 0);
>  
>  	amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
> @@ -343,10 +353,10 @@ static void get_cs_base_and_mask(struct amd64_pvt *pvt, int csrow, u8 dct,
>  		addr_shift	= 4;
>  
>  	/*
> -	* F16h needs two addr_shift values: 8 for high and 6 for low
> -	* (cf. F16h BKDG).
> +	* F16h and F15h(model 30h) needs two addr_shift values:
> +	* 8 for high and 6 for low (cf. F16h BKDG).
>  	*/
> -	} else if (boot_cpu_data.x86 == 0x16) {
> +	} else if (boot_cpu_data.x86 == 0x16 || F15H_M30H_CPU) {

pvt->family and ->model please. This macro is ugly and confusing.

>  		csbase          = pvt->csels[dct].csbases[csrow];
>  		csmask          = pvt->csels[dct].csmasks[csrow >> 1];
>  
> @@ -743,6 +753,9 @@ static void prep_chip_selects(struct amd64_pvt *pvt)
>  	if (boot_cpu_data.x86 == 0xf && pvt->ext_model < K8_REV_F) {
>  		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>  		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 8;
> +	} else if (F15H_M30H_CPU) {
> +		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
> +		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
>  	} else {
>  		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>  		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
> @@ -850,9 +863,9 @@ static u64 get_error_address(struct mce *m)
>  	addr = m->addr & GENMASK(start_bit, end_bit);
>  
>  	/*
> -	 * Erratum 637 workaround
> +	 * Erratum 637 workaround for F15h Models 0x00 - 0x0f

Same here - RG says this erratum is, like 505, "No fix planned"

>  	 */
> -	if (c->x86 == 0x15) {
> +	if (c->x86 == 0x15 && c->x86_model != 0x30) {
>  		struct amd64_pvt *pvt;
>  		u64 cc6_base, tmp_addr;
>  		u32 tmp;
> @@ -918,6 +931,7 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
>  	struct amd_northbridge *nb;
>  	struct pci_dev *misc, *f1 = NULL;
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
> +	unsigned int pci_func;
>  	int off = range << 3;
>  	u32 llim;
>  
> @@ -942,7 +956,12 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
>  		return;
>  
>  	misc = nb->misc;
> -	f1 = pci_get_related_function(misc->vendor, PCI_DEVICE_ID_AMD_15H_NB_F1, misc);
> +
> +	pci_func = (c->x86_model == 0x30) ? PCI_DEVICE_ID_AMD_15H_M30H_NB_F1
> +					  : PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;

Something's wrong with this line :-)

> +
> +	f1 = pci_get_related_function(misc->vendor, pci_func, misc);
> +
>  	if (WARN_ON(!f1))
>  		return;
>  
> @@ -1173,7 +1192,8 @@ static int f15_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
>  }
>  
>  /*
> - * F16h has only limited cs_modes
> + * F16h has only limited cs_modes.
> + * F15 M30h follows the same pattern.
>   */
>  static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
>  				unsigned cs_mode)
> @@ -1218,6 +1238,51 @@ static void read_dram_ctl_register(struct amd64_pvt *pvt)
>  }
>  
>  /*
> + * Determine channel (DCT) based on the interleaving mode:
> + * F15h M30h BKDG, 2.10.12 Memory Interleaving Modes.
> + */
> +static u8 f15_m30h_determine_channel(struct amd64_pvt *pvt, u64 sys_addr,
> +				u8 intlv_en, int num_dcts_intlv, u32 dct_sel)

Arg indentation.

> +{
> +	u8 channel = 0;
> +	u8 select;
> +	u8 intlv_addr = dct_sel_interleave_addr(pvt);
> +
> +	if (!(intlv_en))
> +		return (u8)(dct_sel);
> +	/*
> +	* If num_dcts_intlv == 2, them consider addr bit 8 or addr bit 9
> +	* depending on intlv_addr, then return either channel = 0/1/2/3.
> +	* If num_dcts_intlv == 4, consider either bits[8:9] or bits[9:10]
> +	* depending on intlv_addr, then return the value obtained.

No need for that comment.

> +	*/
> +
> +	if (num_dcts_intlv == 2) {
> +		if (intlv_addr == 0x4)
> +			select = (sys_addr >> 8) & BIT(0);
> +		else if (intlv_addr == 0x5)
> +			select = (sys_addr >> 9) & BIT(0);

So, basically you can take care of the two cases together by doing:

	select = (sys_addr >> 8) & 0x3;

?

> +		else
> +			return -EINVAL;
> +
> +		/* If select !=0, upper DCT selected, else lower DCT */

I think we can see that, no need for the comment.

> +		channel = select ? 0x3 : 0;
> +
> +	} else if (num_dcts_intlv == 4) {
> +		if (intlv_addr == 0x4)
> +			select =  (sys_addr >> 8) & 0x3;
> +		else if (intlv_addr == 0x5)
> +			select = (sys_addr >> 9) & 0x3;

Ditto here, as above.

> +		else
> +			return -EINVAL;
> +
> +		channel = select;
> +	}
> +
> +	return channel;
> +}
> +
> +/*
>   * Determine channel (DCT) based on the interleaving mode: F10h BKDG, 2.8.9 Memory
>   * Interleaving Modes.
>   */
> @@ -1366,6 +1431,10 @@ static int f1x_lookup_addr_in_dct(u64 in_addr, u8 nid, u8 dct)
>  			 (in_addr & cs_mask), (cs_base & cs_mask));
>  
>  		if ((in_addr & cs_mask) == (cs_base & cs_mask)) {
> +			if (F15H_M30H_CPU) {
> +				cs_found =  csrow;
> +				break;
> +			}
>  			cs_found = f10_process_possible_spare(pvt, dct, csrow);
>  
>  			edac_dbg(1, " MATCH csrow=%d\n", cs_found);
> @@ -1492,20 +1561,151 @@ static int f1x_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
>  	return cs_found;
>  }
>  
> -static int f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt, u64 sys_addr,
> -				       int *chan_sel)
> +/*
> + * Routing DRAM Requests algorithm is different for F15h M30h.
> + * It is cleaner to use a brand new function rather than
> + * adding quirks in the more generic 'f1x_match_to_this_node'.
> + * Refer "2.10.5 DRAM Routing Requests" in BKDG for info.
> + */

This comment belongs in the commit message of this patch.

> +static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
> +				  u64 sys_addr, int *chan_sel)

Arg indentation

> +{
> +	int cs_found = -EINVAL;
> +	int num_dcts_intlv = 0;
> +	u64 chan_addr, chan_offset, high_addr_offset;
> +	u64 dct_base, dct_limit;
> +	u32 dct_cont_base_reg, dct_cont_limit_reg, tmp;
> +	u8 channel, alias_channel, leg_mmio_hole;
> +
> +
> +	/*  Read DRAM Control registers specific to F15 M30h. */

No need for the comment.

> +	amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &dct_cont_base_reg);
> +	amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &dct_cont_limit_reg);
> +
> +	/* Extract F15h M30h specific config */

No need for the comment.

> +	u64 dhar_offset		= f10_dhar_offset(pvt);
> +	u8 dct_offset_en	= (u8) ((dct_cont_base_reg >> 3) & BIT(0));
> +	u8 dct_sel		= (u8) ((dct_cont_base_reg >> 4) & 0x7);
> +	u8 intlv_addr		= dct_sel_interleave_addr(pvt);
> +	u8 node_id		= dram_dst_node(pvt, range);
> +	u8 intlv_en		= dram_intlv_en(pvt, range);
> +
> +	edac_dbg(1, "(range %d) SystemAddr= 0x%llx Limit=0x%llx\n",
> +		 range, sys_addr, get_dram_limit(pvt, range));
> +
> +	if (!(get_dram_base(pvt, range)  <= sys_addr) &&
> +	    !(get_dram_limit(pvt, range) >= sys_addr))
> +		return -EINVAL;
> +
> +	if (dhar_valid(pvt) &&
> +	    dhar_base(pvt) <= sys_addr &&
> +	    sys_addr < BIT_64(32)) {
> +		amd64_warn("Huh? Address is in the MMIO hole: 0x%016llx\n",
> +			    sys_addr);
> +		return -EINVAL;
> +	}
> +
> +	/* Verify sys_addr is within DCT Range. */
> +	dct_base = (dct_sel_baseaddr(pvt) << 27);
> +	dct_limit = (((dct_cont_limit_reg >> 11) & 0x1FFF) << 27) | 0x7FFFFFF;
> +	if (!(dct_cont_base_reg & BIT(0)) &&
> +	    !(dct_base <= sys_addr && dct_limit >= sys_addr)) {
> +		return -EINVAL;
> +	}
> +
> +	/* Verify number of dct's that participate in channel interleaving. */
> +	num_dcts_intlv = (int) hweight8(intlv_en);
> +
> +	if (!(num_dcts_intlv % 2 == 0) || (num_dcts_intlv > 4))
> +		return -EINVAL;
> +
> +	channel = f15_m30h_determine_channel(pvt, sys_addr, intlv_en,
> +					     num_dcts_intlv, dct_sel);
> +
> +	/* Verify if we stay within the MAX number of channels allowed */

s/if//

> +	if (channel > 4 || channel < 0)
> +		return -EINVAL;
> +
> +	leg_mmio_hole = (u8) (dct_cont_base_reg >> 1 & BIT(0));
> +
> +	/* Get normalized DCT addr */
> +	if (leg_mmio_hole && (sys_addr >= BIT_64(32)))
> +		chan_offset = dhar_offset;
> +	else
> +		chan_offset = dct_base;
> +
> +	chan_addr = sys_addr - chan_offset;
> +
> +	/* remove channel interleave */
> +	if (num_dcts_intlv == 2) {
> +		if (intlv_addr == 0x4)
> +			chan_addr = ((chan_addr >> 9) << 8) |
> +						(chan_addr & 0xff);
> +		else if (intlv_addr == 0x5)
> +			chan_addr = ((chan_addr >> 10) << 9) |
> +						(chan_addr & 0x1ff);
> +		else
> +			return -EINVAL;
> +
> +	} else if (num_dcts_intlv == 4) {
> +		if (intlv_addr == 0x4)
> +			chan_addr = ((chan_addr >> 10) << 8) |
> +							(chan_addr & 0xff);
> +		else if (intlv_addr == 0x5)
> +			chan_addr = ((chan_addr >> 11) << 9) |
> +							(chan_addr & 0x1ff);
> +		else
> +			return -EINVAL;
> +	}
> +
> +	if (dct_offset_en) {
> +		amd64_read_pci_cfg(pvt->F1,
> +				   DRAM_CONT_HIGH_OFF + (int) channel * 4,
> +				   &tmp);
> +		chan_addr +=  ((tmp >> 11) & 0xfff) << 27;
> +	}
> +
> +	/* Set DCTCFGSEL = ChannelSelect */

No need for the comment.

> +	f15h_select_dct(pvt, channel);
> +
> +	edac_dbg(1, "   Normalized DCT addr: 0x%llx\n", chan_addr);
> +	/* Find the Chip select.. */
> +	/*
> +	 * NOTE: if channel = 3, then alias it to 1.
> +	 * This is because, in F15 M30h, there is support for
> +	 * 4 DCT's, but only 2 are currently included in the architecture.
> +	 * They are DCT0 and DCT3. But we have read all registers of DCT3
> +	 * into pvt->csels[1]. So we need to use '1' here to get correct
> +	 * info. Refer F15 M30h BKDG Section 2.10 and 2.10.3 for clarifications
> +	 */

Comment needs block formatting.

> +
> +	alias_channel =  (channel == 3) ? 1 : channel;
> +
> +	cs_found = f1x_lookup_addr_in_dct(chan_addr, node_id, alias_channel);
> +
> +	if (cs_found >= 0)
> +		*chan_sel = alias_channel;
> +
> +	return cs_found;
> +}
> +
> +static int
> +f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt,
> +			    u64 sys_addr,
> +			    int *chan_sel)
>  {

Arg indentation.

>  	int cs_found = -EINVAL;
>  	unsigned range;
>  
>  	for (range = 0; range < DRAM_RANGES; range++) {
> -
>  		if (!dram_rw(pvt, range))
>  			continue;
> -
> -		if ((get_dram_base(pvt, range)  <= sys_addr) &&
> -		    (get_dram_limit(pvt, range) >= sys_addr)) {
> -
> +		if (F15H_M30H_CPU)
> +			cs_found = f15_m30h_match_to_this_node(pvt, range,
> +							       sys_addr,
> +							       chan_sel);
> +		else if ((get_dram_base(pvt, range)  <= sys_addr) &&
> +			 (get_dram_limit(pvt, range) >= sys_addr)) {
>  			cs_found = f1x_match_to_this_node(pvt, range,
>  							  sys_addr, chan_sel);
>  			if (cs_found >= 0)
> @@ -1624,6 +1824,17 @@ static struct amd64_family_type amd64_family_types[] = {
>  			.read_dct_pci_cfg	= f15_read_dct_pci_cfg,
>  		}
>  	},
> +	[F15_M30H_CPUS] = {
> +		.ctl_name = "F15h_M30h",
> +		.f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1,
> +		.f3_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F3,
> +		.ops = {
> +			.early_channel_count	= f1x_early_channel_count,
> +			.map_sysaddr_to_csrow	= f1x_map_sysaddr_to_csrow,
> +			.dbam_to_cs		= f16_dbam_to_chip_select,
> +			.read_dct_pci_cfg	= f15_read_dct_pci_cfg,
> +		}
> +	},
>  	[F16_CPUS] = {
>  		.ctl_name = "F16h",
>  		.f1_id = PCI_DEVICE_ID_AMD_16H_NB_F1,
> @@ -2388,27 +2599,44 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci,
>  static struct amd64_family_type *amd64_per_family_init(struct amd64_pvt *pvt)
>  {
>  	u8 fam = boot_cpu_data.x86;
> +	u8 model = boot_cpu_data.x86_model;
>  	struct amd64_family_type *fam_type = NULL;
>  
>  	switch (fam) {
>  	case 0xf:
>  		fam_type		= &amd64_family_types[K8_CPUS];
>  		pvt->ops		= &amd64_family_types[K8_CPUS].ops;
> +		pvt->family		= fam;

You could call it pvt->fam for less typing if you like.

> +		pvt->model		= model;
>  		break;
>  
>  	case 0x10:
>  		fam_type		= &amd64_family_types[F10_CPUS];
>  		pvt->ops		= &amd64_family_types[F10_CPUS].ops;
> +		pvt->family		= fam;
> +		pvt->model		= model;
>  		break;
>  
>  	case 0x15:
> +		if (model == 0x30) {
> +			fam_type	= &amd64_family_types[F15_M30H_CPUS];
> +			pvt->ops	= &amd64_family_types[F15_M30H_CPUS].ops;
> +			pvt->family	= fam;
> +			pvt->model	= model;
> +			break;
> +		}
> +
>  		fam_type		= &amd64_family_types[F15_CPUS];
>  		pvt->ops		= &amd64_family_types[F15_CPUS].ops;
> +		pvt->family		= fam;
> +		pvt->model		= model;
>  		break;
>  
>  	case 0x16:
>  		fam_type		= &amd64_family_types[F16_CPUS];
>  		pvt->ops		= &amd64_family_types[F16_CPUS].ops;
> +		pvt->family		= fam;
> +		pvt->model		= model;

This ->family and ->model assignment is repeated in every case. What
do you think, could it probably be done only once, outside the switch
statement?

>  		break;
>  
>  	default:
> @@ -2638,6 +2866,14 @@ static DEFINE_PCI_DEVICE_TABLE(amd64_pci_table) = {
>  	},
>  	{
>  		.vendor		= PCI_VENDOR_ID_AMD,
> +		.device		= PCI_DEVICE_ID_AMD_15H_M30H_NB_F2,
> +		.subvendor	= PCI_ANY_ID,
> +		.subdevice	= PCI_ANY_ID,
> +		.class		= 0,
> +		.class_mask	= 0,
> +	},
> +	{
> +		.vendor		= PCI_VENDOR_ID_AMD,
>  		.device		= PCI_DEVICE_ID_AMD_16H_NB_F2,
>  		.subvendor	= PCI_ANY_ID,
>  		.subdevice	= PCI_ANY_ID,
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 2c6f113..ff15f14 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -170,6 +170,8 @@
>  /*
>   * PCI-defined configuration space registers
>   */
> +#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F1 0x141b
> +#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F2 0x141c
>  #define PCI_DEVICE_ID_AMD_15H_NB_F1	0x1601
>  #define PCI_DEVICE_ID_AMD_15H_NB_F2	0x1602
>  #define PCI_DEVICE_ID_AMD_16H_NB_F1	0x1531
> @@ -181,13 +183,24 @@
>  #define DRAM_BASE_LO			0x40
>  #define DRAM_LIMIT_LO			0x44
>  
> -#define dram_intlv_en(pvt, i)		((u8)((pvt->ranges[i].base.lo >> 8) & 0x7))
> +/*
> + * F15 M30h DRAM Controller Base/Limit
> + * D18F1x2[1C:00]
> + */
> +#define DRAM_CONT_BASE			0x200
> +#define DRAM_CONT_LIMIT			0x204
> +
> +/*
> + * F15 M30h DRAM Controller High Addre Offset
> + * D18F1x2[4C:40]
> + */
> +#define DRAM_CONT_HIGH_OFF      0x240
> +
>  #define dram_rw(pvt, i)			((u8)(pvt->ranges[i].base.lo & 0x3))
>  #define dram_intlv_sel(pvt, i)		((u8)((pvt->ranges[i].lim.lo >> 8) & 0x7))
>  #define dram_dst_node(pvt, i)		((u8)(pvt->ranges[i].lim.lo & 0x7))
>  
>  #define DHAR				0xf0
> -#define dhar_valid(pvt)			((pvt)->dhar & BIT(0))
>  #define dhar_mem_hoist_valid(pvt)	((pvt)->dhar & BIT(1))
>  #define dhar_base(pvt)			((pvt)->dhar & 0xff000000)
>  #define k8_dhar_offset(pvt)		(((pvt)->dhar & 0x0000ff00) << 16)
> @@ -234,8 +247,6 @@
>  #define DDR3_MODE			BIT(8)
>  
>  #define DCT_SEL_LO			0x110
> -#define dct_sel_baseaddr(pvt)		((pvt)->dct_sel_lo & 0xFFFFF800)
> -#define dct_sel_interleave_addr(pvt)	(((pvt)->dct_sel_lo >> 6) & 0x3)
>  #define dct_high_range_enabled(pvt)	((pvt)->dct_sel_lo & BIT(0))
>  #define dct_interleave_enabled(pvt)	((pvt)->dct_sel_lo & BIT(2))
>  
> @@ -293,10 +304,14 @@
>  /* MSRs */
>  #define MSR_MCGCTL_NBE			BIT(4)
>  
> +/* Helper Macro for F15h M30h */
> +#define F15H_M30H_CPU			((pvt->family == 0x15) && \
> +					 (pvt->model == 0x30))

Please drop this macro - the condition is not that complicated to write
out and not error prone.

>  enum amd_families {
>  	K8_CPUS = 0,
>  	F10_CPUS,
>  	F15_CPUS,
> +	F15_M30H_CPUS,
>  	F16_CPUS,
>  	NUM_FAMILIES,
>  };
> @@ -337,6 +352,8 @@ struct amd64_pvt {
>  	struct pci_dev *F1, *F2, *F3;
>  
>  	u16 mc_node_id;		/* MC index of this MC node */
> +	u8 family;		/* CPU family */
> +	u8 model;		/* CPU model */
>  	int ext_model;		/* extended model value of this node */
>  	int channel_count;
>  
> @@ -414,6 +431,14 @@ static inline u16 extract_syndrome(u64 status)
>  	return ((status >> 47) & 0xff) | ((status >> 16) & 0xff00);
>  }
>  
> +static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
> +{
> +	if (F15H_M30H_CPU)
> +		return (((pvt->dct_sel_hi >> 9) & 0x1) << 2) |
> +			((pvt->dct_sel_lo >> 6) & 0x3);
> +
> +	return	((pvt)->dct_sel_lo >> 6) & 0x3;
> +}
>  /*
>   * per-node ECC settings descriptor
>   */
> @@ -504,3 +529,33 @@ static inline void enable_caches(void *dummy)
>  {
>  	write_cr0(read_cr0() & ~X86_CR0_CD);
>  }
> +
> +static inline u8 dram_intlv_en(struct amd64_pvt *pvt, unsigned int i)
> +{
> +	u32 tmp;
> +	if (F15H_M30H_CPU) {

u32 tmp inside the if-statement.

> +		amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &tmp);
> +		return (u8) tmp & 0xF;
> +	}
> +	return (u8) (pvt->ranges[i].base.lo >> 8) & 0x7;
> +}
> +
> +static inline u8 dhar_valid(struct amd64_pvt *pvt)
> +{
> +	u32 tmp;
> +	if (F15H_M30H_CPU) {

ditto.

> +		amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
> +		return (tmp >> 1) & BIT(0);
> +	}
> +	return (pvt)->dhar & BIT(0);
> +}
> +
> +static inline u32 dct_sel_baseaddr(struct amd64_pvt *pvt)
> +{
> +	u32 tmp;
> +	if (F15H_M30H_CPU) {

ditto.

> +		amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
> +		return (tmp >> 11) & 0x1FFF;
> +	}
> +	return (pvt)->dct_sel_lo & 0xFFFFF800;
> +}

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/3 V2] EDAC, AMD64_EDAC: Add ECC decoding support for newer F15h models.

[Aravind Gopalakrishnan]

On 8/6/2013 3:23 PM, Borislav Petkov wrote:
> On Fri, Aug 02, 2013 at 05:43:04PM -0500, Aravind Gopalakrishnan wrote:
>> Adding support for handling ECC error decoding for new F15 models.
>> On newer models, support has been included for upto 4 DCT's,
>> however, only DCT0 and DCT3 are currently configured. (Refer BKDG Section 2.10)
>> There is also a new "Routing DRAM Requests" algorithm for this model.
>>
>> Tested on Fam15h M30h with ECC turned on using mce_amd_inj facility and
>> verified to be functionally correct.
>>
>> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>
>> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
>> index 8b6a034..42dab12 100644
>> --- a/drivers/edac/amd64_edac.c
>> +++ b/drivers/edac/amd64_edac.c
>> @@ -123,7 +123,11 @@ static void f15h_select_dct(struct amd64_pvt *pvt, u8 dct)
>>   	u32 reg = 0;
>>   
>>   	amd64_read_pci_cfg(pvt->F1, DCT_CFG_SEL, &reg);
>> -	reg &= 0xfffffffe;
>> +	/*
>> +	 * If Fam15h M30h, mask out last two bits for programming dct.
>> +	 * Else, only mask out the last bit.
>> +	 */
> No need for that comment.
>
>> +	reg &= (F15_M30H_CPUS) ? 0xfffffffc : 0xfffffff8;
> Let's see, I had 0xfffffffe and now you have 0xfffffff8. See the
> difference?
>
> Also, F15H_M30H_CPUS is an enum, AFAICT, and it will always be true as
> long as it is != 0. What you meant here is "F15H_M30H_CPU". So you see
> how those defines are misleading and made you use them wrong.
>
> So let's correct it all and make it more clear:
>
> 	reg &= (pvt->model >= 0x30) ? ~3 : ~1;
>
> we don't need to look at the family because this function -
> f15h_select_dct - is called on F15h only anyway.
Oops.. My bad. Will fix this.

>>   	reg |= dct;
>>   	amd64_write_pci_cfg(pvt->F1, DCT_CFG_SEL, reg);
>>   }
>> @@ -133,8 +137,12 @@ static int f15_read_dct_pci_cfg(struct amd64_pvt *pvt, int addr, u32 *val,
>>   {
>>   	u8 dct  = 0;
>>   
>> +	/*
>> +	 * For F15 M30h, the second dct is DCT 3.
>> +	 * Refer BKDG Section 2.10
>> +	 */
>>   	if (addr >= 0x140 && addr <= 0x1a0) {
>> -		dct   = 1;
>> +		dct = F15H_M30H_CPU ? 3 : 1;
> ditto here: use pvt->model like above.
Okay.

>>   		addr -= 0x100;
>>   	}
>>   
>> @@ -205,8 +213,9 @@ static int amd64_set_scrub_rate(struct mem_ctl_info *mci, u32 bw)
>>   	if (boot_cpu_data.x86 == 0xf)
>>   		min_scrubrate = 0x0;
>>   
>> -	/* F15h Erratum #505 */
>> -	if (boot_cpu_data.x86 == 0x15)
>> +	/* F15h Models 0x00 - 0x0f Erratum #505 */
> Hmm, are you sure this erratum is fixed in SR? Talk to Eric about it
> first, please. RG says "No fix planned".
>
>> +	if (boot_cpu_data.x86 == 0x15 &&
>> +		boot_cpu_data.x86_model != 0x30)
>>   		f15h_select_dct(pvt, 0);
>>   
>>   	return __amd64_set_scrub_rate(pvt->F3, bw, min_scrubrate);
>> @@ -218,8 +227,9 @@ static int amd64_get_scrub_rate(struct mem_ctl_info *mci)
>>   	u32 scrubval = 0;
>>   	int i, retval = -EINVAL;
>>   
>> -	/* F15h Erratum #505 */
>> -	if (boot_cpu_data.x86 == 0x15)
>> +	/* F15h Models 0x00 - 0x0f Erratum #505 */
> ditto.

I have pinged some people about it, will let you know..

>> +	if (boot_cpu_data.x86 == 0x15 &&
>> +		boot_cpu_data.x86_model != 0x30)
>>   		f15h_select_dct(pvt, 0);
>>   
>>   	amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
>> @@ -343,10 +353,10 @@ static void get_cs_base_and_mask(struct amd64_pvt *pvt, int csrow, u8 dct,
>>   		addr_shift	= 4;
>>   
>>   	/*
>> -	* F16h needs two addr_shift values: 8 for high and 6 for low
>> -	* (cf. F16h BKDG).
>> +	* F16h and F15h(model 30h) needs two addr_shift values:
>> +	* 8 for high and 6 for low (cf. F16h BKDG).
>>   	*/
>> -	} else if (boot_cpu_data.x86 == 0x16) {
>> +	} else if (boot_cpu_data.x86 == 0x16 || F15H_M30H_CPU) {
> pvt->family and ->model please. This macro is ugly and confusing.
>
>>   		csbase          = pvt->csels[dct].csbases[csrow];
>>   		csmask          = pvt->csels[dct].csmasks[csrow >> 1];
>>   
>> @@ -743,6 +753,9 @@ static void prep_chip_selects(struct amd64_pvt *pvt)
>>   	if (boot_cpu_data.x86 == 0xf && pvt->ext_model < K8_REV_F) {
>>   		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>>   		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 8;
>> +	} else if (F15H_M30H_CPU) {
>> +		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
>> +		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
>>   	} else {
>>   		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>>   		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
>> @@ -850,9 +863,9 @@ static u64 get_error_address(struct mce *m)
>>   	addr = m->addr & GENMASK(start_bit, end_bit);
>>   
>>   	/*
>> -	 * Erratum 637 workaround
>> +	 * Erratum 637 workaround for F15h Models 0x00 - 0x0f
> Same here - RG says this erratum is, like 505, "No fix planned"
>
>>   	 */
>> -	if (c->x86 == 0x15) {
>> +	if (c->x86 == 0x15 && c->x86_model != 0x30) {
>>   		struct amd64_pvt *pvt;
>>   		u64 cc6_base, tmp_addr;
>>   		u32 tmp;
>> @@ -918,6 +931,7 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
>>   	struct amd_northbridge *nb;
>>   	struct pci_dev *misc, *f1 = NULL;
>>   	struct cpuinfo_x86 *c = &boot_cpu_data;
>> +	unsigned int pci_func;
>>   	int off = range << 3;
>>   	u32 llim;
>>   
>> @@ -942,7 +956,12 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
>>   		return;
>>   
>>   	misc = nb->misc;
>> -	f1 = pci_get_related_function(misc->vendor, PCI_DEVICE_ID_AMD_15H_NB_F1, misc);
>> +
>> +	pci_func = (c->x86_model == 0x30) ? PCI_DEVICE_ID_AMD_15H_M30H_NB_F1
>> +					  : PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
> Something's wrong with this line :-)

Oh no! (vim auto-complete! :( )
Will fix this.

>> +
>> +	f1 = pci_get_related_function(misc->vendor, pci_func, misc);
>> +
>>   	if (WARN_ON(!f1))
>>   		return;
>>   
>> @@ -1173,7 +1192,8 @@ static int f15_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
>>   }
>>   
>>   /*
>> - * F16h has only limited cs_modes
>> + * F16h has only limited cs_modes.
>> + * F15 M30h follows the same pattern.
>>    */
>>   static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
>>   				unsigned cs_mode)
>> @@ -1218,6 +1238,51 @@ static void read_dram_ctl_register(struct amd64_pvt *pvt)
>>   }
>>   
>>   /*
>> + * Determine channel (DCT) based on the interleaving mode:
>> + * F15h M30h BKDG, 2.10.12 Memory Interleaving Modes.
>> + */
>> +static u8 f15_m30h_determine_channel(struct amd64_pvt *pvt, u64 sys_addr,
>> +				u8 intlv_en, int num_dcts_intlv, u32 dct_sel)
> Arg indentation.
Sorry, will fix.
>> +{
>> +	u8 channel = 0;
>> +	u8 select;
>> +	u8 intlv_addr = dct_sel_interleave_addr(pvt);
>> +
>> +	if (!(intlv_en))
>> +		return (u8)(dct_sel);
>> +	/*
>> +	* If num_dcts_intlv == 2, them consider addr bit 8 or addr bit 9
>> +	* depending on intlv_addr, then return either channel = 0/1/2/3.
>> +	* If num_dcts_intlv == 4, consider either bits[8:9] or bits[9:10]
>> +	* depending on intlv_addr, then return the value obtained.
> No need for that comment.
>
>> +	*/
>> +
>> +	if (num_dcts_intlv == 2) {
>> +		if (intlv_addr == 0x4)
>> +			select = (sys_addr >> 8) & BIT(0);
>> +		else if (intlv_addr == 0x5)
>> +			select = (sys_addr >> 9) & BIT(0);
> So, basically you can take care of the two cases together by doing:
>
> 	select = (sys_addr >> 8) & 0x3;
>
> ?
>
>> +		else
>> +			return -EINVAL;
>> +
>> +		/* If select !=0, upper DCT selected, else lower DCT */
> I think we can see that, no need for the comment.
>
>> +		channel = select ? 0x3 : 0;
>> +
>> +	} else if (num_dcts_intlv == 4) {
>> +		if (intlv_addr == 0x4)
>> +			select =  (sys_addr >> 8) & 0x3;
>> +		else if (intlv_addr == 0x5)
>> +			select = (sys_addr >> 9) & 0x3;
> Ditto here, as above.
>
>> +		else
>> +			return -EINVAL;
>> +
>> +		channel = select;
>> +	}
>> +
>> +	return channel;
>> +}
>> +
>> +/*
>>    * Determine channel (DCT) based on the interleaving mode: F10h BKDG, 2.8.9 Memory
>>    * Interleaving Modes.
>>    */
>> @@ -1366,6 +1431,10 @@ static int f1x_lookup_addr_in_dct(u64 in_addr, u8 nid, u8 dct)
>>   			 (in_addr & cs_mask), (cs_base & cs_mask));
>>   
>>   		if ((in_addr & cs_mask) == (cs_base & cs_mask)) {
>> +			if (F15H_M30H_CPU) {
>> +				cs_found =  csrow;
>> +				break;
>> +			}
>>   			cs_found = f10_process_possible_spare(pvt, dct, csrow);
>>   
>>   			edac_dbg(1, " MATCH csrow=%d\n", cs_found);
>> @@ -1492,20 +1561,151 @@ static int f1x_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
>>   	return cs_found;
>>   }
>>   
>> -static int f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt, u64 sys_addr,
>> -				       int *chan_sel)
>> +/*
>> + * Routing DRAM Requests algorithm is different for F15h M30h.
>> + * It is cleaner to use a brand new function rather than
>> + * adding quirks in the more generic 'f1x_match_to_this_node'.
>> + * Refer "2.10.5 DRAM Routing Requests" in BKDG for info.
>> + */
> This comment belongs in the commit message of this patch.
Okay.

>> +static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
>> +				  u64 sys_addr, int *chan_sel)
> Arg indentation
Sorry, will fix..
>> +{
>> +	int cs_found = -EINVAL;
>> +	int num_dcts_intlv = 0;
>> +	u64 chan_addr, chan_offset, high_addr_offset;
>> +	u64 dct_base, dct_limit;
>> +	u32 dct_cont_base_reg, dct_cont_limit_reg, tmp;
>> +	u8 channel, alias_channel, leg_mmio_hole;
>> +
>> +
>> +	/*  Read DRAM Control registers specific to F15 M30h. */
> No need for the comment.
>
>> +	amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &dct_cont_base_reg);
>> +	amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &dct_cont_limit_reg);
>> +
>> +	/* Extract F15h M30h specific config */
> No need for the comment.
>
>> +	u64 dhar_offset		= f10_dhar_offset(pvt);
>> +	u8 dct_offset_en	= (u8) ((dct_cont_base_reg >> 3) & BIT(0));
>> +	u8 dct_sel		= (u8) ((dct_cont_base_reg >> 4) & 0x7);
>> +	u8 intlv_addr		= dct_sel_interleave_addr(pvt);
>> +	u8 node_id		= dram_dst_node(pvt, range);
>> +	u8 intlv_en		= dram_intlv_en(pvt, range);
>> +
>> +	edac_dbg(1, "(range %d) SystemAddr= 0x%llx Limit=0x%llx\n",
>> +		 range, sys_addr, get_dram_limit(pvt, range));
>> +
>> +	if (!(get_dram_base(pvt, range)  <= sys_addr) &&
>> +	    !(get_dram_limit(pvt, range) >= sys_addr))
>> +		return -EINVAL;
>> +
>> +	if (dhar_valid(pvt) &&
>> +	    dhar_base(pvt) <= sys_addr &&
>> +	    sys_addr < BIT_64(32)) {
>> +		amd64_warn("Huh? Address is in the MMIO hole: 0x%016llx\n",
>> +			    sys_addr);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Verify sys_addr is within DCT Range. */
>> +	dct_base = (dct_sel_baseaddr(pvt) << 27);
>> +	dct_limit = (((dct_cont_limit_reg >> 11) & 0x1FFF) << 27) | 0x7FFFFFF;
>> +	if (!(dct_cont_base_reg & BIT(0)) &&
>> +	    !(dct_base <= sys_addr && dct_limit >= sys_addr)) {
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Verify number of dct's that participate in channel interleaving. */
>> +	num_dcts_intlv = (int) hweight8(intlv_en);
>> +
>> +	if (!(num_dcts_intlv % 2 == 0) || (num_dcts_intlv > 4))
>> +		return -EINVAL;
>> +
>> +	channel = f15_m30h_determine_channel(pvt, sys_addr, intlv_en,
>> +					     num_dcts_intlv, dct_sel);
>> +
>> +	/* Verify if we stay within the MAX number of channels allowed */
> s/if//
>
>> +	if (channel > 4 || channel < 0)
>> +		return -EINVAL;
>> +
>> +	leg_mmio_hole = (u8) (dct_cont_base_reg >> 1 & BIT(0));
>> +
>> +	/* Get normalized DCT addr */
>> +	if (leg_mmio_hole && (sys_addr >= BIT_64(32)))
>> +		chan_offset = dhar_offset;
>> +	else
>> +		chan_offset = dct_base;
>> +
>> +	chan_addr = sys_addr - chan_offset;
>> +
>> +	/* remove channel interleave */
>> +	if (num_dcts_intlv == 2) {
>> +		if (intlv_addr == 0x4)
>> +			chan_addr = ((chan_addr >> 9) << 8) |
>> +						(chan_addr & 0xff);
>> +		else if (intlv_addr == 0x5)
>> +			chan_addr = ((chan_addr >> 10) << 9) |
>> +						(chan_addr & 0x1ff);
>> +		else
>> +			return -EINVAL;
>> +
>> +	} else if (num_dcts_intlv == 4) {
>> +		if (intlv_addr == 0x4)
>> +			chan_addr = ((chan_addr >> 10) << 8) |
>> +							(chan_addr & 0xff);
>> +		else if (intlv_addr == 0x5)
>> +			chan_addr = ((chan_addr >> 11) << 9) |
>> +							(chan_addr & 0x1ff);
>> +		else
>> +			return -EINVAL;
>> +	}
>> +
>> +	if (dct_offset_en) {
>> +		amd64_read_pci_cfg(pvt->F1,
>> +				   DRAM_CONT_HIGH_OFF + (int) channel * 4,
>> +				   &tmp);
>> +		chan_addr +=  ((tmp >> 11) & 0xfff) << 27;
>> +	}
>> +
>> +	/* Set DCTCFGSEL = ChannelSelect */
> No need for the comment.
>
>> +	f15h_select_dct(pvt, channel);
>> +
>> +	edac_dbg(1, "   Normalized DCT addr: 0x%llx\n", chan_addr);
>> +	/* Find the Chip select.. */
>> +	/*
>> +	 * NOTE: if channel = 3, then alias it to 1.
>> +	 * This is because, in F15 M30h, there is support for
>> +	 * 4 DCT's, but only 2 are currently included in the architecture.
>> +	 * They are DCT0 and DCT3. But we have read all registers of DCT3
>> +	 * into pvt->csels[1]. So we need to use '1' here to get correct
>> +	 * info. Refer F15 M30h BKDG Section 2.10 and 2.10.3 for clarifications
>> +	 */
> Comment needs block formatting.
Okay.

>> +
>> +	alias_channel =  (channel == 3) ? 1 : channel;
>> +
>> +	cs_found = f1x_lookup_addr_in_dct(chan_addr, node_id, alias_channel);
>> +
>> +	if (cs_found >= 0)
>> +		*chan_sel = alias_channel;
>> +
>> +	return cs_found;
>> +}
>> +
>> +static int
>> +f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt,
>> +			    u64 sys_addr,
>> +			    int *chan_sel)
>>   {
> Arg indentation.
>
>>   	int cs_found = -EINVAL;
>>   	unsigned range;
>>   
>>   	for (range = 0; range < DRAM_RANGES; range++) {
>> -
>>   		if (!dram_rw(pvt, range))
>>   			continue;
>> -
>> -		if ((get_dram_base(pvt, range)  <= sys_addr) &&
>> -		    (get_dram_limit(pvt, range) >= sys_addr)) {
>> -
>> +		if (F15H_M30H_CPU)
>> +			cs_found = f15_m30h_match_to_this_node(pvt, range,
>> +							       sys_addr,
>> +							       chan_sel);
>> +		else if ((get_dram_base(pvt, range)  <= sys_addr) &&
>> +			 (get_dram_limit(pvt, range) >= sys_addr)) {
>>   			cs_found = f1x_match_to_this_node(pvt, range,
>>   							  sys_addr, chan_sel);
>>   			if (cs_found >= 0)
>> @@ -1624,6 +1824,17 @@ static struct amd64_family_type amd64_family_types[] = {
>>   			.read_dct_pci_cfg	= f15_read_dct_pci_cfg,
>>   		}
>>   	},
>> +	[F15_M30H_CPUS] = {
>> +		.ctl_name = "F15h_M30h",
>> +		.f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1,
>> +		.f3_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F3,
>> +		.ops = {
>> +			.early_channel_count	= f1x_early_channel_count,
>> +			.map_sysaddr_to_csrow	= f1x_map_sysaddr_to_csrow,
>> +			.dbam_to_cs		= f16_dbam_to_chip_select,
>> +			.read_dct_pci_cfg	= f15_read_dct_pci_cfg,
>> +		}
>> +	},
>>   	[F16_CPUS] = {
>>   		.ctl_name = "F16h",
>>   		.f1_id = PCI_DEVICE_ID_AMD_16H_NB_F1,
>> @@ -2388,27 +2599,44 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci,
>>   static struct amd64_family_type *amd64_per_family_init(struct amd64_pvt *pvt)
>>   {
>>   	u8 fam = boot_cpu_data.x86;
>> +	u8 model = boot_cpu_data.x86_model;
>>   	struct amd64_family_type *fam_type = NULL;
>>   
>>   	switch (fam) {
>>   	case 0xf:
>>   		fam_type		= &amd64_family_types[K8_CPUS];
>>   		pvt->ops		= &amd64_family_types[K8_CPUS].ops;
>> +		pvt->family		= fam;
> You could call it pvt->fam for less typing if you like.
Sure!, and I will take it outside the switch as suggested below.
>> +		pvt->model		= model;
>>   		break;
>>   
>>   	case 0x10:
>>   		fam_type		= &amd64_family_types[F10_CPUS];
>>   		pvt->ops		= &amd64_family_types[F10_CPUS].ops;
>> +		pvt->family		= fam;
>> +		pvt->model		= model;
>>   		break;
>>   
>>   	case 0x15:
>> +		if (model == 0x30) {
>> +			fam_type	= &amd64_family_types[F15_M30H_CPUS];
>> +			pvt->ops	= &amd64_family_types[F15_M30H_CPUS].ops;
>> +			pvt->family	= fam;
>> +			pvt->model	= model;
>> +			break;
>> +		}
>> +
>>   		fam_type		= &amd64_family_types[F15_CPUS];
>>   		pvt->ops		= &amd64_family_types[F15_CPUS].ops;
>> +		pvt->family		= fam;
>> +		pvt->model		= model;
>>   		break;
>>   
>>   	case 0x16:
>>   		fam_type		= &amd64_family_types[F16_CPUS];
>>   		pvt->ops		= &amd64_family_types[F16_CPUS].ops;
>> +		pvt->family		= fam;
>> +		pvt->model		= model;
> This ->family and ->model assignment is repeated in every case. What
> do you think, could it probably be done only once, outside the switch
> statement?
>
>>   		break;
>>   
>>   	default:
>> @@ -2638,6 +2866,14 @@ static DEFINE_PCI_DEVICE_TABLE(amd64_pci_table) = {
>>   	},
>>   	{
>>   		.vendor		= PCI_VENDOR_ID_AMD,
>> +		.device		= PCI_DEVICE_ID_AMD_15H_M30H_NB_F2,
>> +		.subvendor	= PCI_ANY_ID,
>> +		.subdevice	= PCI_ANY_ID,
>> +		.class		= 0,
>> +		.class_mask	= 0,
>> +	},
>> +	{
>> +		.vendor		= PCI_VENDOR_ID_AMD,
>>   		.device		= PCI_DEVICE_ID_AMD_16H_NB_F2,
>>   		.subvendor	= PCI_ANY_ID,
>>   		.subdevice	= PCI_ANY_ID,
>> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
>> index 2c6f113..ff15f14 100644
>> --- a/drivers/edac/amd64_edac.h
>> +++ b/drivers/edac/amd64_edac.h
>> @@ -170,6 +170,8 @@
>>   /*
>>    * PCI-defined configuration space registers
>>    */
>> +#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F1 0x141b
>> +#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F2 0x141c
>>   #define PCI_DEVICE_ID_AMD_15H_NB_F1	0x1601
>>   #define PCI_DEVICE_ID_AMD_15H_NB_F2	0x1602
>>   #define PCI_DEVICE_ID_AMD_16H_NB_F1	0x1531
>> @@ -181,13 +183,24 @@
>>   #define DRAM_BASE_LO			0x40
>>   #define DRAM_LIMIT_LO			0x44
>>   
>> -#define dram_intlv_en(pvt, i)		((u8)((pvt->ranges[i].base.lo >> 8) & 0x7))
>> +/*
>> + * F15 M30h DRAM Controller Base/Limit
>> + * D18F1x2[1C:00]
>> + */
>> +#define DRAM_CONT_BASE			0x200
>> +#define DRAM_CONT_LIMIT			0x204
>> +
>> +/*
>> + * F15 M30h DRAM Controller High Addre Offset
>> + * D18F1x2[4C:40]
>> + */
>> +#define DRAM_CONT_HIGH_OFF      0x240
>> +
>>   #define dram_rw(pvt, i)			((u8)(pvt->ranges[i].base.lo & 0x3))
>>   #define dram_intlv_sel(pvt, i)		((u8)((pvt->ranges[i].lim.lo >> 8) & 0x7))
>>   #define dram_dst_node(pvt, i)		((u8)(pvt->ranges[i].lim.lo & 0x7))
>>   
>>   #define DHAR				0xf0
>> -#define dhar_valid(pvt)			((pvt)->dhar & BIT(0))
>>   #define dhar_mem_hoist_valid(pvt)	((pvt)->dhar & BIT(1))
>>   #define dhar_base(pvt)			((pvt)->dhar & 0xff000000)
>>   #define k8_dhar_offset(pvt)		(((pvt)->dhar & 0x0000ff00) << 16)
>> @@ -234,8 +247,6 @@
>>   #define DDR3_MODE			BIT(8)
>>   
>>   #define DCT_SEL_LO			0x110
>> -#define dct_sel_baseaddr(pvt)		((pvt)->dct_sel_lo & 0xFFFFF800)
>> -#define dct_sel_interleave_addr(pvt)	(((pvt)->dct_sel_lo >> 6) & 0x3)
>>   #define dct_high_range_enabled(pvt)	((pvt)->dct_sel_lo & BIT(0))
>>   #define dct_interleave_enabled(pvt)	((pvt)->dct_sel_lo & BIT(2))
>>   
>> @@ -293,10 +304,14 @@
>>   /* MSRs */
>>   #define MSR_MCGCTL_NBE			BIT(4)
>>   
>> +/* Helper Macro for F15h M30h */
>> +#define F15H_M30H_CPU			((pvt->family == 0x15) && \
>> +					 (pvt->model == 0x30))
> Please drop this macro - the condition is not that complicated to write
> out and not error prone.
Okay.
>>   enum amd_families {
>>   	K8_CPUS = 0,
>>   	F10_CPUS,
>>   	F15_CPUS,
>> +	F15_M30H_CPUS,
>>   	F16_CPUS,
>>   	NUM_FAMILIES,
>>   };
>> @@ -337,6 +352,8 @@ struct amd64_pvt {
>>   	struct pci_dev *F1, *F2, *F3;
>>   
>>   	u16 mc_node_id;		/* MC index of this MC node */
>> +	u8 family;		/* CPU family */
>> +	u8 model;		/* CPU model */
>>   	int ext_model;		/* extended model value of this node */
>>   	int channel_count;
>>   
>> @@ -414,6 +431,14 @@ static inline u16 extract_syndrome(u64 status)
>>   	return ((status >> 47) & 0xff) | ((status >> 16) & 0xff00);
>>   }
>>   
>> +static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
>> +{
>> +	if (F15H_M30H_CPU)
>> +		return (((pvt->dct_sel_hi >> 9) & 0x1) << 2) |
>> +			((pvt->dct_sel_lo >> 6) & 0x3);
>> +
>> +	return	((pvt)->dct_sel_lo >> 6) & 0x3;
>> +}
>>   /*
>>    * per-node ECC settings descriptor
>>    */
>> @@ -504,3 +529,33 @@ static inline void enable_caches(void *dummy)
>>   {
>>   	write_cr0(read_cr0() & ~X86_CR0_CD);
>>   }
>> +
>> +static inline u8 dram_intlv_en(struct amd64_pvt *pvt, unsigned int i)
>> +{
>> +	u32 tmp;
>> +	if (F15H_M30H_CPU) {
> u32 tmp inside the if-statement.
>
>> +		amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &tmp);
>> +		return (u8) tmp & 0xF;
>> +	}
>> +	return (u8) (pvt->ranges[i].base.lo >> 8) & 0x7;
>> +}
>> +
>> +static inline u8 dhar_valid(struct amd64_pvt *pvt)
>> +{
>> +	u32 tmp;
>> +	if (F15H_M30H_CPU) {
> ditto.
>
>> +		amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
>> +		return (tmp >> 1) & BIT(0);
>> +	}
>> +	return (pvt)->dhar & BIT(0);
>> +}
>> +
>> +static inline u32 dct_sel_baseaddr(struct amd64_pvt *pvt)
>> +{
>> +	u32 tmp;
>> +	if (F15H_M30H_CPU) {
> ditto.
>
>> +		amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
>> +		return (tmp >> 11) & 0x1FFF;
>> +	}
>> +	return (pvt)->dct_sel_lo & 0xFFFFF800;
>> +}

will send out changes in V3;

Thanks,
Aravind.

Re: [PATCH 3/3 V2] EDAC, AMD64_EDAC: Add ECC decoding support for newer F15h models.

[Aravind Gopalakrishnan]

On 8/6/2013 3:55 PM, Aravind Gopalakrishnan wrote:
> On 8/6/2013 3:23 PM, Borislav Petkov wrote:
>> On Fri, Aug 02, 2013 at 05:43:04PM -0500, Aravind Gopalakrishnan wrote:
>>> Adding support for handling ECC error decoding for new F15 models.
>>> On newer models, support has been included for upto 4 DCT's,
>>> however, only DCT0 and DCT3 are currently configured. (Refer BKDG 
>>> Section 2.10)
>>> There is also a new "Routing DRAM Requests" algorithm for this model.
>>>
>>> Tested on Fam15h M30h with ECC turned on using mce_amd_inj facility and
>>> verified to be functionally correct.
>>>
>>> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>>
>>> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
>>> index 8b6a034..42dab12 100644
>>> --- a/drivers/edac/amd64_edac.c
>>> +++ b/drivers/edac/amd64_edac.c
>>> @@ -123,7 +123,11 @@ static void f15h_select_dct(struct amd64_pvt 
>>> *pvt, u8 dct)
>>>       u32 reg = 0;
>>>         amd64_read_pci_cfg(pvt->F1, DCT_CFG_SEL, &reg);
>>> -    reg &= 0xfffffffe;
>>> +    /*
>>> +     * If Fam15h M30h, mask out last two bits for programming dct.
>>> +     * Else, only mask out the last bit.
>>> +     */
>> No need for that comment.
>>
>>> +    reg &= (F15_M30H_CPUS) ? 0xfffffffc : 0xfffffff8;
>> Let's see, I had 0xfffffffe and now you have 0xfffffff8. See the
>> difference?
>>
>> Also, F15H_M30H_CPUS is an enum, AFAICT, and it will always be true as
>> long as it is != 0. What you meant here is "F15H_M30H_CPU". So you see
>> how those defines are misleading and made you use them wrong.
>>
>> So let's correct it all and make it more clear:
>>
>>     reg &= (pvt->model >= 0x30) ? ~3 : ~1;
>>
>> we don't need to look at the family because this function -
>> f15h_select_dct - is called on F15h only anyway.
> Oops.. My bad. Will fix this.
>
>>>       reg |= dct;
>>>       amd64_write_pci_cfg(pvt->F1, DCT_CFG_SEL, reg);
>>>   }
>>> @@ -133,8 +137,12 @@ static int f15_read_dct_pci_cfg(struct 
>>> amd64_pvt *pvt, int addr, u32 *val,
>>>   {
>>>       u8 dct  = 0;
>>>   +    /*
>>> +     * For F15 M30h, the second dct is DCT 3.
>>> +     * Refer BKDG Section 2.10
>>> +     */
>>>       if (addr >= 0x140 && addr <= 0x1a0) {
>>> -        dct   = 1;
>>> +        dct = F15H_M30H_CPU ? 3 : 1;
>> ditto here: use pvt->model like above.
> Okay.
>
>>>           addr -= 0x100;
>>>       }
>>>   @@ -205,8 +213,9 @@ static int amd64_set_scrub_rate(struct 
>>> mem_ctl_info *mci, u32 bw)
>>>       if (boot_cpu_data.x86 == 0xf)
>>>           min_scrubrate = 0x0;
>>>   -    /* F15h Erratum #505 */
>>> -    if (boot_cpu_data.x86 == 0x15)
>>> +    /* F15h Models 0x00 - 0x0f Erratum #505 */
>> Hmm, are you sure this erratum is fixed in SR? Talk to Eric about it
>> first, please. RG says "No fix planned".
>>
>>> +    if (boot_cpu_data.x86 == 0x15 &&
>>> +        boot_cpu_data.x86_model != 0x30)
>>>           f15h_select_dct(pvt, 0);
>>>         return __amd64_set_scrub_rate(pvt->F3, bw, min_scrubrate);
>>> @@ -218,8 +227,9 @@ static int amd64_get_scrub_rate(struct 
>>> mem_ctl_info *mci)
>>>       u32 scrubval = 0;
>>>       int i, retval = -EINVAL;
>>>   -    /* F15h Erratum #505 */
>>> -    if (boot_cpu_data.x86 == 0x15)
>>> +    /* F15h Models 0x00 - 0x0f Erratum #505 */
>> ditto.
>
> I have pinged some people about it, will let you know..
>
>>> +    if (boot_cpu_data.x86 == 0x15 &&
>>> +        boot_cpu_data.x86_model != 0x30)
>>>           f15h_select_dct(pvt, 0);
>>>         amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
>>> @@ -343,10 +353,10 @@ static void get_cs_base_and_mask(struct 
>>> amd64_pvt *pvt, int csrow, u8 dct,
>>>           addr_shift    = 4;
>>>         /*
>>> -    * F16h needs two addr_shift values: 8 for high and 6 for low
>>> -    * (cf. F16h BKDG).
>>> +    * F16h and F15h(model 30h) needs two addr_shift values:
>>> +    * 8 for high and 6 for low (cf. F16h BKDG).
>>>       */
>>> -    } else if (boot_cpu_data.x86 == 0x16) {
>>> +    } else if (boot_cpu_data.x86 == 0x16 || F15H_M30H_CPU) {
>> pvt->family and ->model please. This macro is ugly and confusing.
>>

Quick question:
Shall I change all instances of boot_cpu_data.[x86|x86_model] to use 
pvt->fam and pvt->model
wherever applicable as part of this patch or have it go in as a separate 
patch?

>>>           csbase          = pvt->csels[dct].csbases[csrow];
>>>           csmask          = pvt->csels[dct].csmasks[csrow >> 1];
>>>   @@ -743,6 +753,9 @@ static void prep_chip_selects(struct amd64_pvt 
>>> *pvt)
>>>       if (boot_cpu_data.x86 == 0xf && pvt->ext_model < K8_REV_F) {
>>>           pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>>>           pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 8;
>>> +    } else if (F15H_M30H_CPU) {
>>> +        pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
>>> +        pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
>>>       } else {
>>>           pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>>>           pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
>>> @@ -850,9 +863,9 @@ static u64 get_error_address(struct mce *m)
>>>       addr = m->addr & GENMASK(start_bit, end_bit);
>>>         /*
>>> -     * Erratum 637 workaround
>>> +     * Erratum 637 workaround for F15h Models 0x00 - 0x0f
>> Same here - RG says this erratum is, like 505, "No fix planned"
>>
>>>        */
>>> -    if (c->x86 == 0x15) {
>>> +    if (c->x86 == 0x15 && c->x86_model != 0x30) {
>>>           struct amd64_pvt *pvt;
>>>           u64 cc6_base, tmp_addr;
>>>           u32 tmp;
>>> @@ -918,6 +931,7 @@ static void read_dram_base_limit_regs(struct 
>>> amd64_pvt *pvt, unsigned range)
>>>       struct amd_northbridge *nb;
>>>       struct pci_dev *misc, *f1 = NULL;
>>>       struct cpuinfo_x86 *c = &boot_cpu_data;
>>> +    unsigned int pci_func;
>>>       int off = range << 3;
>>>       u32 llim;
>>>   @@ -942,7 +956,12 @@ static void read_dram_base_limit_regs(struct 
>>> amd64_pvt *pvt, unsigned range)
>>>           return;
>>>         misc = nb->misc;
>>> -    f1 = pci_get_related_function(misc->vendor, 
>>> PCI_DEVICE_ID_AMD_15H_NB_F1, misc);
>>> +
>>> +    pci_func = (c->x86_model == 0x30) ? 
>>> PCI_DEVICE_ID_AMD_15H_M30H_NB_F1
>>> +                      : PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
>> Something's wrong with this line :-)
>
> Oh no! (vim auto-complete! :( )
> Will fix this.
>
>>> +
>>> +    f1 = pci_get_related_function(misc->vendor, pci_func, misc);
>>> +
>>>       if (WARN_ON(!f1))
>>>           return;
>>>   @@ -1173,7 +1192,8 @@ static int f15_dbam_to_chip_select(struct 
>>> amd64_pvt *pvt, u8 dct,
>>>   }
>>>     /*
>>> - * F16h has only limited cs_modes
>>> + * F16h has only limited cs_modes.
>>> + * F15 M30h follows the same pattern.
>>>    */
>>>   static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
>>>                   unsigned cs_mode)
>>> @@ -1218,6 +1238,51 @@ static void read_dram_ctl_register(struct 
>>> amd64_pvt *pvt)
>>>   }
>>>     /*
>>> + * Determine channel (DCT) based on the interleaving mode:
>>> + * F15h M30h BKDG, 2.10.12 Memory Interleaving Modes.
>>> + */
>>> +static u8 f15_m30h_determine_channel(struct amd64_pvt *pvt, u64 
>>> sys_addr,
>>> +                u8 intlv_en, int num_dcts_intlv, u32 dct_sel)
>> Arg indentation.
> Sorry, will fix.
>>> +{
>>> +    u8 channel = 0;
>>> +    u8 select;
>>> +    u8 intlv_addr = dct_sel_interleave_addr(pvt);
>>> +
>>> +    if (!(intlv_en))
>>> +        return (u8)(dct_sel);
>>> +    /*
>>> +    * If num_dcts_intlv == 2, them consider addr bit 8 or addr bit 9
>>> +    * depending on intlv_addr, then return either channel = 0/1/2/3.
>>> +    * If num_dcts_intlv == 4, consider either bits[8:9] or bits[9:10]
>>> +    * depending on intlv_addr, then return the value obtained.
>> No need for that comment.
>>
>>> +    */
>>> +
>>> +    if (num_dcts_intlv == 2) {
>>> +        if (intlv_addr == 0x4)
>>> +            select = (sys_addr >> 8) & BIT(0);
>>> +        else if (intlv_addr == 0x5)
>>> +            select = (sys_addr >> 9) & BIT(0);
>> So, basically you can take care of the two cases together by doing:
>>
>>     select = (sys_addr >> 8) & 0x3;
>>
>> ?
>>
>>> +        else
>>> +            return -EINVAL;
>>> +
>>> +        /* If select !=0, upper DCT selected, else lower DCT */
>> I think we can see that, no need for the comment.
>>
>>> +        channel = select ? 0x3 : 0;
>>> +
>>> +    } else if (num_dcts_intlv == 4) {
>>> +        if (intlv_addr == 0x4)
>>> +            select =  (sys_addr >> 8) & 0x3;
>>> +        else if (intlv_addr == 0x5)
>>> +            select = (sys_addr >> 9) & 0x3;
>> Ditto here, as above.
>>
>>> +        else
>>> +            return -EINVAL;
>>> +
>>> +        channel = select;
>>> +    }
>>> +
>>> +    return channel;
>>> +}
>>> +
>>> +/*
>>>    * Determine channel (DCT) based on the interleaving mode: F10h 
>>> BKDG, 2.8.9 Memory
>>>    * Interleaving Modes.
>>>    */
>>> @@ -1366,6 +1431,10 @@ static int f1x_lookup_addr_in_dct(u64 
>>> in_addr, u8 nid, u8 dct)
>>>                (in_addr & cs_mask), (cs_base & cs_mask));
>>>             if ((in_addr & cs_mask) == (cs_base & cs_mask)) {
>>> +            if (F15H_M30H_CPU) {
>>> +                cs_found =  csrow;
>>> +                break;
>>> +            }
>>>               cs_found = f10_process_possible_spare(pvt, dct, csrow);
>>>                 edac_dbg(1, " MATCH csrow=%d\n", cs_found);
>>> @@ -1492,20 +1561,151 @@ static int f1x_match_to_this_node(struct 
>>> amd64_pvt *pvt, unsigned range,
>>>       return cs_found;
>>>   }
>>>   -static int f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt, u64 
>>> sys_addr,
>>> -                       int *chan_sel)
>>> +/*
>>> + * Routing DRAM Requests algorithm is different for F15h M30h.
>>> + * It is cleaner to use a brand new function rather than
>>> + * adding quirks in the more generic 'f1x_match_to_this_node'.
>>> + * Refer "2.10.5 DRAM Routing Requests" in BKDG for info.
>>> + */
>> This comment belongs in the commit message of this patch.
> Okay.
>
>>> +static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, 
>>> unsigned range,
>>> +                  u64 sys_addr, int *chan_sel)
>> Arg indentation
> Sorry, will fix..
>>> +{
>>> +    int cs_found = -EINVAL;
>>> +    int num_dcts_intlv = 0;
>>> +    u64 chan_addr, chan_offset, high_addr_offset;
>>> +    u64 dct_base, dct_limit;
>>> +    u32 dct_cont_base_reg, dct_cont_limit_reg, tmp;
>>> +    u8 channel, alias_channel, leg_mmio_hole;
>>> +
>>> +
>>> +    /*  Read DRAM Control registers specific to F15 M30h. */
>> No need for the comment.
>>
>>> +    amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &dct_cont_base_reg);
>>> +    amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &dct_cont_limit_reg);
>>> +
>>> +    /* Extract F15h M30h specific config */
>> No need for the comment.
>>
>>> +    u64 dhar_offset        = f10_dhar_offset(pvt);
>>> +    u8 dct_offset_en    = (u8) ((dct_cont_base_reg >> 3) & BIT(0));
>>> +    u8 dct_sel        = (u8) ((dct_cont_base_reg >> 4) & 0x7);
>>> +    u8 intlv_addr        = dct_sel_interleave_addr(pvt);
>>> +    u8 node_id        = dram_dst_node(pvt, range);
>>> +    u8 intlv_en        = dram_intlv_en(pvt, range);
>>> +
>>> +    edac_dbg(1, "(range %d) SystemAddr= 0x%llx Limit=0x%llx\n",
>>> +         range, sys_addr, get_dram_limit(pvt, range));
>>> +
>>> +    if (!(get_dram_base(pvt, range)  <= sys_addr) &&
>>> +        !(get_dram_limit(pvt, range) >= sys_addr))
>>> +        return -EINVAL;
>>> +
>>> +    if (dhar_valid(pvt) &&
>>> +        dhar_base(pvt) <= sys_addr &&
>>> +        sys_addr < BIT_64(32)) {
>>> +        amd64_warn("Huh? Address is in the MMIO hole: 0x%016llx\n",
>>> +                sys_addr);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* Verify sys_addr is within DCT Range. */
>>> +    dct_base = (dct_sel_baseaddr(pvt) << 27);
>>> +    dct_limit = (((dct_cont_limit_reg >> 11) & 0x1FFF) << 27) | 
>>> 0x7FFFFFF;
>>> +    if (!(dct_cont_base_reg & BIT(0)) &&
>>> +        !(dct_base <= sys_addr && dct_limit >= sys_addr)) {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* Verify number of dct's that participate in channel 
>>> interleaving. */
>>> +    num_dcts_intlv = (int) hweight8(intlv_en);
>>> +
>>> +    if (!(num_dcts_intlv % 2 == 0) || (num_dcts_intlv > 4))
>>> +        return -EINVAL;
>>> +
>>> +    channel = f15_m30h_determine_channel(pvt, sys_addr, intlv_en,
>>> +                         num_dcts_intlv, dct_sel);
>>> +
>>> +    /* Verify if we stay within the MAX number of channels allowed */
>> s/if//
>>
>>> +    if (channel > 4 || channel < 0)
>>> +        return -EINVAL;
>>> +
>>> +    leg_mmio_hole = (u8) (dct_cont_base_reg >> 1 & BIT(0));
>>> +
>>> +    /* Get normalized DCT addr */
>>> +    if (leg_mmio_hole && (sys_addr >= BIT_64(32)))
>>> +        chan_offset = dhar_offset;
>>> +    else
>>> +        chan_offset = dct_base;
>>> +
>>> +    chan_addr = sys_addr - chan_offset;
>>> +
>>> +    /* remove channel interleave */
>>> +    if (num_dcts_intlv == 2) {
>>> +        if (intlv_addr == 0x4)
>>> +            chan_addr = ((chan_addr >> 9) << 8) |
>>> +                        (chan_addr & 0xff);
>>> +        else if (intlv_addr == 0x5)
>>> +            chan_addr = ((chan_addr >> 10) << 9) |
>>> +                        (chan_addr & 0x1ff);
>>> +        else
>>> +            return -EINVAL;
>>> +
>>> +    } else if (num_dcts_intlv == 4) {
>>> +        if (intlv_addr == 0x4)
>>> +            chan_addr = ((chan_addr >> 10) << 8) |
>>> +                            (chan_addr & 0xff);
>>> +        else if (intlv_addr == 0x5)
>>> +            chan_addr = ((chan_addr >> 11) << 9) |
>>> +                            (chan_addr & 0x1ff);
>>> +        else
>>> +            return -EINVAL;
>>> +    }
>>> +
>>> +    if (dct_offset_en) {
>>> +        amd64_read_pci_cfg(pvt->F1,
>>> +                   DRAM_CONT_HIGH_OFF + (int) channel * 4,
>>> +                   &tmp);
>>> +        chan_addr +=  ((tmp >> 11) & 0xfff) << 27;
>>> +    }
>>> +
>>> +    /* Set DCTCFGSEL = ChannelSelect */
>> No need for the comment.
>>
>>> +    f15h_select_dct(pvt, channel);
>>> +
>>> +    edac_dbg(1, "   Normalized DCT addr: 0x%llx\n", chan_addr);
>>> +    /* Find the Chip select.. */
>>> +    /*
>>> +     * NOTE: if channel = 3, then alias it to 1.
>>> +     * This is because, in F15 M30h, there is support for
>>> +     * 4 DCT's, but only 2 are currently included in the architecture.
>>> +     * They are DCT0 and DCT3. But we have read all registers of DCT3
>>> +     * into pvt->csels[1]. So we need to use '1' here to get correct
>>> +     * info. Refer F15 M30h BKDG Section 2.10 and 2.10.3 for 
>>> clarifications
>>> +     */
>> Comment needs block formatting.
> Okay.
>
>>> +
>>> +    alias_channel =  (channel == 3) ? 1 : channel;
>>> +
>>> +    cs_found = f1x_lookup_addr_in_dct(chan_addr, node_id, 
>>> alias_channel);
>>> +
>>> +    if (cs_found >= 0)
>>> +        *chan_sel = alias_channel;
>>> +
>>> +    return cs_found;
>>> +}
>>> +
>>> +static int
>>> +f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt,
>>> +                u64 sys_addr,
>>> +                int *chan_sel)
>>>   {
>> Arg indentation.
>>
>>>       int cs_found = -EINVAL;
>>>       unsigned range;
>>>         for (range = 0; range < DRAM_RANGES; range++) {
>>> -
>>>           if (!dram_rw(pvt, range))
>>>               continue;
>>> -
>>> -        if ((get_dram_base(pvt, range)  <= sys_addr) &&
>>> -            (get_dram_limit(pvt, range) >= sys_addr)) {
>>> -
>>> +        if (F15H_M30H_CPU)
>>> +            cs_found = f15_m30h_match_to_this_node(pvt, range,
>>> +                                   sys_addr,
>>> +                                   chan_sel);
>>> +        else if ((get_dram_base(pvt, range)  <= sys_addr) &&
>>> +             (get_dram_limit(pvt, range) >= sys_addr)) {
>>>               cs_found = f1x_match_to_this_node(pvt, range,
>>>                                 sys_addr, chan_sel);
>>>               if (cs_found >= 0)
>>> @@ -1624,6 +1824,17 @@ static struct amd64_family_type 
>>> amd64_family_types[] = {
>>>               .read_dct_pci_cfg    = f15_read_dct_pci_cfg,
>>>           }
>>>       },
>>> +    [F15_M30H_CPUS] = {
>>> +        .ctl_name = "F15h_M30h",
>>> +        .f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1,
>>> +        .f3_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F3,
>>> +        .ops = {
>>> +            .early_channel_count    = f1x_early_channel_count,
>>> +            .map_sysaddr_to_csrow    = f1x_map_sysaddr_to_csrow,
>>> +            .dbam_to_cs        = f16_dbam_to_chip_select,
>>> +            .read_dct_pci_cfg    = f15_read_dct_pci_cfg,
>>> +        }
>>> +    },
>>>       [F16_CPUS] = {
>>>           .ctl_name = "F16h",
>>>           .f1_id = PCI_DEVICE_ID_AMD_16H_NB_F1,
>>> @@ -2388,27 +2599,44 @@ static void setup_mci_misc_attrs(struct 
>>> mem_ctl_info *mci,
>>>   static struct amd64_family_type *amd64_per_family_init(struct 
>>> amd64_pvt *pvt)
>>>   {
>>>       u8 fam = boot_cpu_data.x86;
>>> +    u8 model = boot_cpu_data.x86_model;
>>>       struct amd64_family_type *fam_type = NULL;
>>>         switch (fam) {
>>>       case 0xf:
>>>           fam_type        = &amd64_family_types[K8_CPUS];
>>>           pvt->ops        = &amd64_family_types[K8_CPUS].ops;
>>> +        pvt->family        = fam;
>> You could call it pvt->fam for less typing if you like.
> Sure!, and I will take it outside the switch as suggested below.
>>> +        pvt->model        = model;
>>>           break;
>>>         case 0x10:
>>>           fam_type        = &amd64_family_types[F10_CPUS];
>>>           pvt->ops        = &amd64_family_types[F10_CPUS].ops;
>>> +        pvt->family        = fam;
>>> +        pvt->model        = model;
>>>           break;
>>>         case 0x15:
>>> +        if (model == 0x30) {
>>> +            fam_type    = &amd64_family_types[F15_M30H_CPUS];
>>> +            pvt->ops    = &amd64_family_types[F15_M30H_CPUS].ops;
>>> +            pvt->family    = fam;
>>> +            pvt->model    = model;
>>> +            break;
>>> +        }
>>> +
>>>           fam_type        = &amd64_family_types[F15_CPUS];
>>>           pvt->ops        = &amd64_family_types[F15_CPUS].ops;
>>> +        pvt->family        = fam;
>>> +        pvt->model        = model;
>>>           break;
>>>         case 0x16:
>>>           fam_type        = &amd64_family_types[F16_CPUS];
>>>           pvt->ops        = &amd64_family_types[F16_CPUS].ops;
>>> +        pvt->family        = fam;
>>> +        pvt->model        = model;
>> This ->family and ->model assignment is repeated in every case. What
>> do you think, could it probably be done only once, outside the switch
>> statement?
>>
>>>           break;
>>>         default:
>>> @@ -2638,6 +2866,14 @@ static 
>>> DEFINE_PCI_DEVICE_TABLE(amd64_pci_table) = {
>>>       },
>>>       {
>>>           .vendor        = PCI_VENDOR_ID_AMD,
>>> +        .device        = PCI_DEVICE_ID_AMD_15H_M30H_NB_F2,
>>> +        .subvendor    = PCI_ANY_ID,
>>> +        .subdevice    = PCI_ANY_ID,
>>> +        .class        = 0,
>>> +        .class_mask    = 0,
>>> +    },
>>> +    {
>>> +        .vendor        = PCI_VENDOR_ID_AMD,
>>>           .device        = PCI_DEVICE_ID_AMD_16H_NB_F2,
>>>           .subvendor    = PCI_ANY_ID,
>>>           .subdevice    = PCI_ANY_ID,
>>> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
>>> index 2c6f113..ff15f14 100644
>>> --- a/drivers/edac/amd64_edac.h
>>> +++ b/drivers/edac/amd64_edac.h
>>> @@ -170,6 +170,8 @@
>>>   /*
>>>    * PCI-defined configuration space registers
>>>    */
>>> +#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F1 0x141b
>>> +#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F2 0x141c
>>>   #define PCI_DEVICE_ID_AMD_15H_NB_F1    0x1601
>>>   #define PCI_DEVICE_ID_AMD_15H_NB_F2    0x1602
>>>   #define PCI_DEVICE_ID_AMD_16H_NB_F1    0x1531
>>> @@ -181,13 +183,24 @@
>>>   #define DRAM_BASE_LO            0x40
>>>   #define DRAM_LIMIT_LO            0x44
>>>   -#define dram_intlv_en(pvt, i) ((u8)((pvt->ranges[i].base.lo >> 8) 
>>> & 0x7))
>>> +/*
>>> + * F15 M30h DRAM Controller Base/Limit
>>> + * D18F1x2[1C:00]
>>> + */
>>> +#define DRAM_CONT_BASE            0x200
>>> +#define DRAM_CONT_LIMIT            0x204
>>> +
>>> +/*
>>> + * F15 M30h DRAM Controller High Addre Offset
>>> + * D18F1x2[4C:40]
>>> + */
>>> +#define DRAM_CONT_HIGH_OFF      0x240
>>> +
>>>   #define dram_rw(pvt, i) ((u8)(pvt->ranges[i].base.lo & 0x3))
>>>   #define dram_intlv_sel(pvt, i) ((u8)((pvt->ranges[i].lim.lo >> 8) 
>>> & 0x7))
>>>   #define dram_dst_node(pvt, i) ((u8)(pvt->ranges[i].lim.lo & 0x7))
>>>     #define DHAR                0xf0
>>> -#define dhar_valid(pvt)            ((pvt)->dhar & BIT(0))
>>>   #define dhar_mem_hoist_valid(pvt)    ((pvt)->dhar & BIT(1))
>>>   #define dhar_base(pvt)            ((pvt)->dhar & 0xff000000)
>>>   #define k8_dhar_offset(pvt)        (((pvt)->dhar & 0x0000ff00) << 16)
>>> @@ -234,8 +247,6 @@
>>>   #define DDR3_MODE            BIT(8)
>>>     #define DCT_SEL_LO            0x110
>>> -#define dct_sel_baseaddr(pvt)        ((pvt)->dct_sel_lo & 0xFFFFF800)
>>> -#define dct_sel_interleave_addr(pvt) (((pvt)->dct_sel_lo >> 6) & 0x3)
>>>   #define dct_high_range_enabled(pvt)    ((pvt)->dct_sel_lo & BIT(0))
>>>   #define dct_interleave_enabled(pvt)    ((pvt)->dct_sel_lo & BIT(2))
>>>   @@ -293,10 +304,14 @@
>>>   /* MSRs */
>>>   #define MSR_MCGCTL_NBE            BIT(4)
>>>   +/* Helper Macro for F15h M30h */
>>> +#define F15H_M30H_CPU            ((pvt->family == 0x15) && \
>>> +                     (pvt->model == 0x30))
>> Please drop this macro - the condition is not that complicated to write
>> out and not error prone.
> Okay.
>>>   enum amd_families {
>>>       K8_CPUS = 0,
>>>       F10_CPUS,
>>>       F15_CPUS,
>>> +    F15_M30H_CPUS,
>>>       F16_CPUS,
>>>       NUM_FAMILIES,
>>>   };
>>> @@ -337,6 +352,8 @@ struct amd64_pvt {
>>>       struct pci_dev *F1, *F2, *F3;
>>>         u16 mc_node_id;        /* MC index of this MC node */
>>> +    u8 family;        /* CPU family */
>>> +    u8 model;        /* CPU model */
>>>       int ext_model;        /* extended model value of this node */
>>>       int channel_count;
>>>   @@ -414,6 +431,14 @@ static inline u16 extract_syndrome(u64 status)
>>>       return ((status >> 47) & 0xff) | ((status >> 16) & 0xff00);
>>>   }
>>>   +static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
>>> +{
>>> +    if (F15H_M30H_CPU)
>>> +        return (((pvt->dct_sel_hi >> 9) & 0x1) << 2) |
>>> +            ((pvt->dct_sel_lo >> 6) & 0x3);
>>> +
>>> +    return    ((pvt)->dct_sel_lo >> 6) & 0x3;
>>> +}
>>>   /*
>>>    * per-node ECC settings descriptor
>>>    */
>>> @@ -504,3 +529,33 @@ static inline void enable_caches(void *dummy)
>>>   {
>>>       write_cr0(read_cr0() & ~X86_CR0_CD);
>>>   }
>>> +
>>> +static inline u8 dram_intlv_en(struct amd64_pvt *pvt, unsigned int i)
>>> +{
>>> +    u32 tmp;
>>> +    if (F15H_M30H_CPU) {
>> u32 tmp inside the if-statement.
>>
>>> +        amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &tmp);
>>> +        return (u8) tmp & 0xF;
>>> +    }
>>> +    return (u8) (pvt->ranges[i].base.lo >> 8) & 0x7;
>>> +}
>>> +
>>> +static inline u8 dhar_valid(struct amd64_pvt *pvt)
>>> +{
>>> +    u32 tmp;
>>> +    if (F15H_M30H_CPU) {
>> ditto.
>>
>>> +        amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
>>> +        return (tmp >> 1) & BIT(0);
>>> +    }
>>> +    return (pvt)->dhar & BIT(0);
>>> +}
>>> +
>>> +static inline u32 dct_sel_baseaddr(struct amd64_pvt *pvt)
>>> +{
>>> +    u32 tmp;
>>> +    if (F15H_M30H_CPU) {
>> ditto.
>>
>>> +        amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
>>> +        return (tmp >> 11) & 0x1FFF;
>>> +    }
>>> +    return (pvt)->dct_sel_lo & 0xFFFFF800;
>>> +}
>
> will send out changes in V3;
>
> Thanks,
> Aravind.

Re: [PATCH 3/3 V2] EDAC, AMD64_EDAC: Add ECC decoding support for newer F15h models.

[Borislav Petkov]

On Tue, Aug 06, 2013 at 05:00:51PM -0500, Aravind Gopalakrishnan wrote:
> Quick question: Shall I change all instances of
> boot_cpu_data.[x86|x86_model] to use pvt->fam and pvt->model wherever
> applicable as part of this patch or have it go in as a separate patch?

Yes, a separate pre-patch would be lovely.

Also, please trim your reply by quoting only the relevant part because
with a huge mail, almost completely quoted like this one, I'm choking in
">" lines trying to find the text you wrote.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/3 V2] EDAC, AMD64_EDAC: Add ECC decoding support for newer F15h models.

[Aravind Gopalakrishnan]

>
>>>           addr -= 0x100;
>>>       }
>>>   @@ -205,8 +213,9 @@ static int amd64_set_scrub_rate(struct 
>>> mem_ctl_info *mci, u32 bw)
>>>       if (boot_cpu_data.x86 == 0xf)
>>>           min_scrubrate = 0x0;
>>>   -    /* F15h Erratum #505 */
>>> -    if (boot_cpu_data.x86 == 0x15)
>>> +    /* F15h Models 0x00 - 0x0f Erratum #505 */
>> Hmm, are you sure this erratum is fixed in SR? Talk to Eric about it
>> first, please. RG says "No fix planned".
>>
>>> +    if (boot_cpu_data.x86 == 0x15 &&
>>> +        boot_cpu_data.x86_model != 0x30)
>>>           f15h_select_dct(pvt, 0);
>>>         return __amd64_set_scrub_rate(pvt->F3, bw, min_scrubrate);
>>> @@ -218,8 +227,9 @@ static int amd64_get_scrub_rate(struct 
>>> mem_ctl_info *mci)
>>>       u32 scrubval = 0;
>>>       int i, retval = -EINVAL;
>>>   -    /* F15h Erratum #505 */
>>> -    if (boot_cpu_data.x86 == 0x15)
>>> +    /* F15h Models 0x00 - 0x0f Erratum #505 */
>> ditto.
>
> I have pinged some people about it, will let you know..
>
 From email conversations internally, here are the current status of the 
errata:
* Erratum 505: Should not be carried over to newer Fam15h models.
* Erratum 637: Not fixed.

I have taken this into account and changed the code to workaround only E637.

> will send out changes in V3;
>
> Thanks,
> Aravind.
Corrected the indentations; Removed unnecessary comments;
Sending out changes in  [PATCH 3/3 V3].
Thanks,
-Aravind.