[PATCH 0/3] Updates to amd64_edac and ghes_edac

[PATCH 0/3] Updates to amd64_edac and ghes_edac

[Aravind Gopalakrishnan]

Patch 1: Cleanup patch to remove redundant definiton of memory types.
	 No functional change introduced
Patch 2: For F15hM60h, register used to program scrub rate has changed.
	 Add support in the code for the new register
Patch 3: Update comments section regarding copyright and documentation info
	 No functional change here.

Aravind Gopalakrishnan (3):
  EDAC, ghes_edac: Remove redundant memory_type array
  EDAC, amd64_edac: Extend scrub rate programmability feature for
    F15hM60h
  EDAC, amd64_edac: Update copyright and documentation info

 drivers/edac/amd64_edac.c | 53 ++++++++++++++++++++++++++++++++++++++---------
 drivers/edac/amd64_edac.h | 33 +++++++++++++++++++++++++++--
 drivers/edac/ghes_edac.c  | 22 +-------------------
 3 files changed, 75 insertions(+), 33 deletions(-)

-- 
2.5.0

[PATCH 1/3] EDAC, ghes_edac: Remove redundant memory_type array

[Aravind Gopalakrishnan]

We already have edac_mem_types[] that enumerates the different
kinds of memory. So, use that and remove the redundant
memory_type[] array here.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 drivers/edac/ghes_edac.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index b246819..b1e4512 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -66,26 +66,6 @@ struct ghes_edac_dimm_fill {
 	unsigned count;
 };
 
-char *memory_type[] = {
-	[MEM_EMPTY] = "EMPTY",
-	[MEM_RESERVED] = "RESERVED",
-	[MEM_UNKNOWN] = "UNKNOWN",
-	[MEM_FPM] = "FPM",
-	[MEM_EDO] = "EDO",
-	[MEM_BEDO] = "BEDO",
-	[MEM_SDR] = "SDR",
-	[MEM_RDR] = "RDR",
-	[MEM_DDR] = "DDR",
-	[MEM_RDDR] = "RDDR",
-	[MEM_RMBS] = "RMBS",
-	[MEM_DDR2] = "DDR2",
-	[MEM_FB_DDR2] = "FB_DDR2",
-	[MEM_RDDR2] = "RDDR2",
-	[MEM_XDR] = "XDR",
-	[MEM_DDR3] = "DDR3",
-	[MEM_RDDR3] = "RDDR3",
-};
-
 static void ghes_edac_count_dimms(const struct dmi_header *dh, void *arg)
 {
 	int *num_dimm = arg;
@@ -173,7 +153,7 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
 
 		if (dimm->nr_pages) {
 			edac_dbg(1, "DIMM%i: %s size = %d MB%s\n",
-				dimm_fill->count, memory_type[dimm->mtype],
+				dimm_fill->count, edac_mem_types[dimm->mtype],
 				PAGES_TO_MiB(dimm->nr_pages),
 				(dimm->edac_mode != EDAC_NONE) ? "(ECC)" : "");
 			edac_dbg(2, "\ttype %d, detail 0x%02x, width %d(total %d)\n",
-- 
2.5.0

[PATCH 2/3] EDAC, amd64_edac: Extend scrub rate programmability feature for F15hM60h

[Aravind Gopalakrishnan]

For F15h M60h processor, the scrub rate control register has moved
to F2 of PCI config space and is at a different offset from
earlier processors. The minimun recommended scrub rate is also different.
(Refer D18F2x1c8_dct[1:0][DramScrub] on Fam15hM60h BKDG)

Modify the set_scrub_rate() and get_scrub_rate() functions so that
they are aware of these changes.

Fixing couple of indentation issues since I am touching the file.

Tested on F15hM60h, Fam15h Models 00h-0fh and Fam10h systems and
it works fine.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 drivers/edac/amd64_edac.c | 53 ++++++++++++++++++++++++++++++++++++++---------
 drivers/edac/amd64_edac.h |  3 +++
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 73aea40..1ec4a13 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -173,7 +173,7 @@ static inline int amd64_read_dct_pci_cfg(struct amd64_pvt *pvt, u8 dct,
  * scan the scrub rate mapping table for a close or matching bandwidth value to
  * issue. If requested is too big, then use last maximum value found.
  */
-static int __set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate)
+static u32 find_scrub_rate(u32 new_bw, u32 min_rate, u32 *scrub_bw)
 {
 	u32 scrubval;
 	int i;
@@ -200,28 +200,52 @@ static int __set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate)
 	}
 
 	scrubval = scrubrates[i].scrubval;
+	*scrub_bw = scrubval ? scrubrates[i].bandwidth : 0;
 
-	pci_write_bits32(ctl, SCRCTRL, scrubval, 0x001F);
+	return scrubval;
+}
 
-	if (scrubval)
-		return scrubrates[i].bandwidth;
+static inline void __set_scrub_rate(struct pci_dev *ctl, int offset,
+				    u32 scrubval)
+{
+	pci_write_bits32(ctl, offset, scrubval, SCRMASK);
 
-	return 0;
 }
 
 static int set_scrub_rate(struct mem_ctl_info *mci, u32 bw)
 {
 	struct amd64_pvt *pvt = mci->pvt_info;
 	u32 min_scrubrate = 0x5;
+	u32 scrubrate, scrub_bw;
 
 	if (pvt->fam == 0xf)
 		min_scrubrate = 0x0;
+	else if (pvt->fam == 0x15 && pvt->model == 0x60)
+		min_scrubrate = 0x6;
 
 	/* Erratum #505 */
 	if (pvt->fam == 0x15 && pvt->model < 0x10)
 		f15h_select_dct(pvt, 0);
 
-	return __set_scrub_rate(pvt->F3, bw, min_scrubrate);
+	scrubrate = find_scrub_rate(bw, min_scrubrate, &scrub_bw);
+
+	/* Scrub rate control register moved to F2 register space for
+	 * F15hM60h andit is per DCT now. So, need to select the DCT
+	 * using DCT_CFG_SEL first and then program the scrubrate
+	 */
+	if (pvt->fam == 0x15 && pvt->model == 0x60) {
+		f15h_select_dct(pvt, 0);
+		__set_scrub_rate(pvt->F2, F15H_M60H_SCRCTRL, scrubrate);
+		f15h_select_dct(pvt, 1);
+		__set_scrub_rate(pvt->F2, F15H_M60H_SCRCTRL, scrubrate);
+
+		goto scrub_out;
+	}
+
+	__set_scrub_rate(pvt->F3, SCRCTRL, scrubrate);
+
+scrub_out:
+	return scrub_bw;
 }
 
 static int get_scrub_rate(struct mem_ctl_info *mci)
@@ -234,9 +258,18 @@ static int get_scrub_rate(struct mem_ctl_info *mci)
 	if (pvt->fam == 0x15 && pvt->model < 0x10)
 		f15h_select_dct(pvt, 0);
 
-	amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
+	if (pvt->fam == 0x15 && pvt->model == 0x60) {
+		/* Since we mirror the same scrubrate value across
+		 * both DCTs, it is enough to read the value off one of
+		 * the DCT registers.
+		 */
+		f15h_select_dct(pvt, 0);
+		amd64_read_pci_cfg(pvt->F2, F15H_M60H_SCRCTRL, &scrubval);
+	} else {
+		amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
+	}
 
-	scrubval = scrubval & 0x001F;
+	scrubval = scrubval & SCRMASK;
 
 	for (i = 0; i < ARRAY_SIZE(scrubrates); i++) {
 		if (scrubrates[i].scrubval == scrubval) {
@@ -1316,7 +1349,7 @@ static int f15_m60h_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
  * F16h and F15h model 30h have only limited cs_modes.
  */
 static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
-				unsigned cs_mode, int cs_mask_nr)
+				   unsigned cs_mode, int cs_mask_nr)
 {
 	WARN_ON(cs_mode > 12);
 
@@ -1666,7 +1699,7 @@ static int f1x_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
 }
 
 static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
-					u64 sys_addr, int *chan_sel)
+				       u64 sys_addr, int *chan_sel)
 {
 	int cs_found = -EINVAL;
 	int num_dcts_intlv = 0;
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 4bdec75..15c0eb4 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -255,6 +255,8 @@
 
 #define DCT_SEL_HI			0x114
 
+#define F15H_M60H_SCRCTRL		0x1C8
+
 /*
  * Function 3 - Misc Control
  */
@@ -269,6 +271,7 @@
 #define NBSL_PP_OBS			0x2
 
 #define SCRCTRL				0x58
+#define SCRMASK				0x1F
 
 #define F10_ONLINE_SPARE		0xB0
 #define online_spare_swap_done(pvt, c)	(((pvt)->online_spare >> (1 + 2 * (c))) & 0x1)
-- 
2.5.0

[PATCH 3/3] EDAC, amd64_edac: Update copyright and documentation info

[Aravind Gopalakrishnan]

Extending the comments section to include the BKDG references
for newer processors which this EDAC driver supports.

Updated the copyrights info and fixed a trivial typo while at it.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 drivers/edac/amd64_edac.h | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 15c0eb4..1e34d07 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -2,7 +2,7 @@
  * AMD64 class Memory Controller kernel module
  *
  * Copyright (c) 2009 SoftwareBitMaker.
- * Copyright (c) 2009 Advanced Micro Devices, Inc.
+ * Copyright (c) 2009-15 Advanced Micro Devices, Inc.
  *
  * This file may be distributed under the terms of the
  * GNU General Public License.
@@ -36,13 +36,18 @@
  *	Changes/Fixes by Borislav Petkov <bp@alien8.de>:
  *		- misc fixes and code cleanups
  *
+ *	Changes by Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
+ *		- Add support for-
+ *		   - Fam15h Models (30-3fh; 60-6fh)
+ *		   - Fam16h Models (00-0fh; 30-3fh)
+ *
  * This module is based on the following documents
  * (available from http://www.amd.com/):
  *
  *	Title:	BIOS and Kernel Developer's Guide for AMD Athlon 64 and AMD
  *		Opteron Processors
  *	AMD publication #: 26094
- *`	Revision: 3.26
+ *	Revision: 3.26
  *
  *	Title:	BIOS and Kernel Developer's Guide for AMD NPT Family 0Fh
  *		Processors
@@ -56,6 +61,27 @@
  *	Revision: 3.00
  *	Issue Date: September 07, 2007
  *
+ *	Title: BIOS and Kernel Developer's Guide (BKDG) for AMD Family 15h
+ *	       Models 30h-3Fh Processors
+ *	AMD publication #: 49125
+ *	Revision: 3.06
+ *	Issue Date: 2/12/2015 (latest release)
+ *	Link: http://support.amd.com/TechDocs/49125_15h_Models_30h-3Fh_BKDG.pdf
+ *
+ *	Title: BIOS and Kernel Developer's Guide (BKDG) for AMD Family 15h
+ *	       Models 60h-6Fh Processors
+ *	AMD publication #: 50742
+ *	Revision: 3.01
+ *	Issue Date: 7/23/2015 (latest release)
+ *	Link: http://support.amd.com/TechDocs/50742_15h_Models_60h-6Fh_BKDG.pdf
+ *
+ *	Title: BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h
+ *	       Models 00h-0Fh Processors
+ *	AMD publication #: 48751
+ *	Revision: 3.03
+ *	Issue Date: 2/23/2015 (latest release)
+ *	Link: http://support.amd.com/TechDocs/48751_16h_bkdg.pdf
+ *
  * Sections in the first 2 documents are no longer in sync with each other.
  * The Family 10h BKDG was totally re-written from scratch with a new
  * presentation model.
-- 
2.5.0

Re: [PATCH 3/3] EDAC, amd64_edac: Update copyright and documentation info

[Borislav Petkov]

On Wed, Sep 16, 2015 at 03:53:31PM -0500, Aravind Gopalakrishnan wrote:
> Extending the comments section to include the BKDG references
> for newer processors which this EDAC driver supports.
> 
> Updated the copyrights info and fixed a trivial typo while at it.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  drivers/edac/amd64_edac.h | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 15c0eb4..1e34d07 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -2,7 +2,7 @@
>   * AMD64 class Memory Controller kernel module
>   *
>   * Copyright (c) 2009 SoftwareBitMaker.
> - * Copyright (c) 2009 Advanced Micro Devices, Inc.
> + * Copyright (c) 2009-15 Advanced Micro Devices, Inc.
>   *
>   * This file may be distributed under the terms of the
>   * GNU General Public License.
> @@ -36,13 +36,18 @@
>   *	Changes/Fixes by Borislav Petkov <bp@alien8.de>:
>   *		- misc fixes and code cleanups
>   *
> + *	Changes by Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> + *		- Add support for-
> + *		   - Fam15h Models (30-3fh; 60-6fh)
> + *		   - Fam16h Models (00-0fh; 30-3fh)
> + *

Actually, we have git which can give you all the exact changelog
information you'd need. So instead of adding more to the header, simply
remove all changelog info from there. It is a thing from the past anyway.

>   * This module is based on the following documents
>   * (available from http://www.amd.com/):
>   *
>   *	Title:	BIOS and Kernel Developer's Guide for AMD Athlon 64 and AMD
>   *		Opteron Processors
>   *	AMD publication #: 26094
> - *`	Revision: 3.26
> + *	Revision: 3.26
>   *
>   *	Title:	BIOS and Kernel Developer's Guide for AMD NPT Family 0Fh
>   *		Processors
> @@ -56,6 +61,27 @@
>   *	Revision: 3.00
>   *	Issue Date: September 07, 2007
>   *
> + *	Title: BIOS and Kernel Developer's Guide (BKDG) for AMD Family 15h
> + *	       Models 30h-3Fh Processors
> + *	AMD publication #: 49125
> + *	Revision: 3.06
> + *	Issue Date: 2/12/2015 (latest release)
> + *	Link: http://support.amd.com/TechDocs/49125_15h_Models_30h-3Fh_BKDG.pdf
> + *
> + *	Title: BIOS and Kernel Developer's Guide (BKDG) for AMD Family 15h
> + *	       Models 60h-6Fh Processors
> + *	AMD publication #: 50742
> + *	Revision: 3.01
> + *	Issue Date: 7/23/2015 (latest release)
> + *	Link: http://support.amd.com/TechDocs/50742_15h_Models_60h-6Fh_BKDG.pdf
> + *
> + *	Title: BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h
> + *	       Models 00h-0Fh Processors
> + *	AMD publication #: 48751
> + *	Revision: 3.03
> + *	Issue Date: 2/23/2015 (latest release)
> + *	Link: http://support.amd.com/TechDocs/48751_16h_bkdg.pdf
> + *

You can add those to an amd64_edac section of Documentation/edac.txt.

I want to keep header files as clean as possible. Copyright updates are
fine but everything history-like and unrelated to functionality should
go somewhere else, if at all.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 3/3] EDAC, amd64_edac: Update copyright and documentation info

[Aravind Gopalakrishnan]

On 9/17/15 2:43 AM, Borislav Petkov wrote:
> On Wed, Sep 16, 2015 at 03:53:31PM -0500, Aravind Gopalakrishnan wrote:
>>    *	Changes/Fixes by Borislav Petkov <bp@alien8.de>:
>>    *		- misc fixes and code cleanups
>>    *
>> + *	Changes by Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>> + *		- Add support for-
>> + *		   - Fam15h Models (30-3fh; 60-6fh)
>> + *		   - Fam16h Models (00-0fh; 30-3fh)
>> + *
> Actually, we have git which can give you all the exact changelog
> information you'd need. So instead of adding more to the header, simply
> remove all changelog info from there. It is a thing from the past anyway.

Okay, will do.

>>    *
>> + *	Title: BIOS and Kernel Developer's Guide (BKDG) for AMD Family 15h
>> + *	       Models 30h-3Fh Processors
>> + *	AMD publication #: 49125
>> + *	Revision: 3.06
>> + *	Issue Date: 2/12/2015 (latest release)
>> + *	Link: http://support.amd.com/TechDocs/49125_15h_Models_30h-3Fh_BKDG.pdf
>> + *
>> + *	Title: BIOS and Kernel Developer's Guide (BKDG) for AMD Family 15h
>> + *	       Models 60h-6Fh Processors
>> + *	AMD publication #: 50742
>> + *	Revision: 3.01
>> + *	Issue Date: 7/23/2015 (latest release)
>> + *	Link: http://support.amd.com/TechDocs/50742_15h_Models_60h-6Fh_BKDG.pdf
>> + *
>> + *	Title: BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h
>> + *	       Models 00h-0Fh Processors
>> + *	AMD publication #: 48751
>> + *	Revision: 3.03
>> + *	Issue Date: 2/23/2015 (latest release)
>> + *	Link: http://support.amd.com/TechDocs/48751_16h_bkdg.pdf
>> + *
> You can add those to an amd64_edac section of Documentation/edac.txt.

Okay, I'll look into that.

> I want to keep header files as clean as possible. Copyright updates are
> fine but everything history-like and unrelated to functionality should
> go somewhere else, if at all.
>
>
Shall I send a V2 of this patch to update copyrights and remove the 
changelogs alone?
(I can meld it with patch 2/3 too if you prefer..)

And I can take up the Documentation work as a separate patch..

Thanks,
-Aravind.

Re: [PATCH 3/3] EDAC, amd64_edac: Update copyright and documentation info

[Borislav Petkov]

On Thu, Sep 17, 2015 at 09:35:25AM -0500, Aravind Gopalakrishnan wrote:
> Shall I send a V2 of this patch to update copyrights and remove the
> changelogs alone?
> (I can meld it with patch 2/3 too if you prefer..)

Let me take a look at the rest first. Am on a conf currently so it'll be
next week, most likely.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 3/3] EDAC, amd64_edac: Update copyright and documentation info

[Aravind Gopalakrishnan]

On 9/17/15 10:24 AM, Borislav Petkov wrote:
> On Thu, Sep 17, 2015 at 09:35:25AM -0500, Aravind Gopalakrishnan wrote:
>> Shall I send a V2 of this patch to update copyrights and remove the
>> changelogs alone?
>> (I can meld it with patch 2/3 too if you prefer..)
> Let me take a look at the rest first. Am on a conf currently so it'll be
> next week, most likely.
>

Sure, no problem.

-Aravind.

Re: [PATCH 2/3] EDAC, amd64_edac: Extend scrub rate programmability feature for F15hM60h

[Borislav Petkov]

On Wed, Sep 16, 2015 at 03:53:30PM -0500, Aravind Gopalakrishnan wrote:
> For F15h M60h processor, the scrub rate control register has moved
> to F2 of PCI config space and is at a different offset from
> earlier processors. The minimun recommended scrub rate is also different.
> (Refer D18F2x1c8_dct[1:0][DramScrub] on Fam15hM60h BKDG)
> 
> Modify the set_scrub_rate() and get_scrub_rate() functions so that
> they are aware of these changes.
> 
> Fixing couple of indentation issues since I am touching the file.
> 
> Tested on F15hM60h, Fam15h Models 00h-0fh and Fam10h systems and
> it works fine.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  drivers/edac/amd64_edac.c | 53 ++++++++++++++++++++++++++++++++++++++---------
>  drivers/edac/amd64_edac.h |  3 +++
>  2 files changed, 46 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 73aea40..1ec4a13 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -173,7 +173,7 @@ static inline int amd64_read_dct_pci_cfg(struct amd64_pvt *pvt, u8 dct,
>   * scan the scrub rate mapping table for a close or matching bandwidth value to
>   * issue. If requested is too big, then use last maximum value found.
>   */
> -static int __set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate)
> +static u32 find_scrub_rate(u32 new_bw, u32 min_rate, u32 *scrub_bw)
>  {
>  	u32 scrubval;
>  	int i;
> @@ -200,28 +200,52 @@ static int __set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate)
>  	}
>  
>  	scrubval = scrubrates[i].scrubval;
> +	*scrub_bw = scrubval ? scrubrates[i].bandwidth : 0;
>  
> -	pci_write_bits32(ctl, SCRCTRL, scrubval, 0x001F);
> +	return scrubval;
> +}
>  
> -	if (scrubval)
> -		return scrubrates[i].bandwidth;
> +static inline void __set_scrub_rate(struct pci_dev *ctl, int offset,
> +				    u32 scrubval)
> +{
> +	pci_write_bits32(ctl, offset, scrubval, SCRMASK);
>  
> -	return 0;
>  }
>  

What is all that churn good for?

What's wrong with simply adding the model 0x60 check to
__set_scrub_rate() and doing the proper write there?

>  static int set_scrub_rate(struct mem_ctl_info *mci, u32 bw)
>  {
>  	struct amd64_pvt *pvt = mci->pvt_info;
>  	u32 min_scrubrate = 0x5;
> +	u32 scrubrate, scrub_bw;
>  
>  	if (pvt->fam == 0xf)
>  		min_scrubrate = 0x0;
> +	else if (pvt->fam == 0x15 && pvt->model == 0x60)
> +		min_scrubrate = 0x6;
>  
>  	/* Erratum #505 */
>  	if (pvt->fam == 0x15 && pvt->model < 0x10)
>  		f15h_select_dct(pvt, 0);
>  
> -	return __set_scrub_rate(pvt->F3, bw, min_scrubrate);
> +	scrubrate = find_scrub_rate(bw, min_scrubrate, &scrub_bw);
> +
> +	/* Scrub rate control register moved to F2 register space for
> +	 * F15hM60h andit is per DCT now. So, need to select the DCT
> +	 * using DCT_CFG_SEL first and then program the scrubrate
> +	 */
> +	if (pvt->fam == 0x15 && pvt->model == 0x60) {
> +		f15h_select_dct(pvt, 0);
> +		__set_scrub_rate(pvt->F2, F15H_M60H_SCRCTRL, scrubrate);
> +		f15h_select_dct(pvt, 1);
> +		__set_scrub_rate(pvt->F2, F15H_M60H_SCRCTRL, scrubrate);
> +
> +		goto scrub_out;
> +	}
> +
> +	__set_scrub_rate(pvt->F3, SCRCTRL, scrubrate);
> +
> +scrub_out:
> +	return scrub_bw;
>  }
>  
>  static int get_scrub_rate(struct mem_ctl_info *mci)
> @@ -234,9 +258,18 @@ static int get_scrub_rate(struct mem_ctl_info *mci)
>  	if (pvt->fam == 0x15 && pvt->model < 0x10)
>  		f15h_select_dct(pvt, 0);
>  
> -	amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
> +	if (pvt->fam == 0x15 && pvt->model == 0x60) {
> +		/* Since we mirror the same scrubrate value across
> +		 * both DCTs, it is enough to read the value off one of
> +		 * the DCT registers.
> +		 */
> +		f15h_select_dct(pvt, 0);

If it is enough, why do you select DCT 0? Just read the currently
selected one, whichever it is...

> +		amd64_read_pci_cfg(pvt->F2, F15H_M60H_SCRCTRL, &scrubval);
> +	} else {
> +		amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
> +	}
>  
> -	scrubval = scrubval & 0x001F;
> +	scrubval = scrubval & SCRMASK;
>  
>  	for (i = 0; i < ARRAY_SIZE(scrubrates); i++) {
>  		if (scrubrates[i].scrubval == scrubval) {
> @@ -1316,7 +1349,7 @@ static int f15_m60h_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
>   * F16h and F15h model 30h have only limited cs_modes.
>   */
>  static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
> -				unsigned cs_mode, int cs_mask_nr)
> +				   unsigned cs_mode, int cs_mask_nr)
>  {
>  	WARN_ON(cs_mode > 12);

Why is that hunk here?

> @@ -1666,7 +1699,7 @@ static int f1x_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
>  }
>  
>  static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
> -					u64 sys_addr, int *chan_sel)
> +				       u64 sys_addr, int *chan_sel)
>  {
>  	int cs_found = -EINVAL;
>  	int num_dcts_intlv = 0;

That one too?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 1/3] EDAC, ghes_edac: Remove redundant memory_type array

[Borislav Petkov]

On Wed, Sep 16, 2015 at 03:53:29PM -0500, Aravind Gopalakrishnan wrote:
> We already have edac_mem_types[] that enumerates the different
> kinds of memory. So, use that and remove the redundant
> memory_type[] array here.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  drivers/edac/ghes_edac.c | 22 +---------------------
>  1 file changed, 1 insertion(+), 21 deletions(-)

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 2/3] EDAC, amd64_edac: Extend scrub rate programmability feature for F15hM60h

[Aravind Gopalakrishnan]

On 9/24/2015 4:18 AM, Borislav Petkov wrote:
> On Wed, Sep 16, 2015 at 03:53:30PM -0500, Aravind Gopalakrishnan wrote:
>> -static int __set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate)
>> +static u32 find_scrub_rate(u32 new_bw, u32 min_rate, u32 *scrub_bw)
>>   {
>>   	u32 scrubval;
>>   	int i;
>> @@ -200,28 +200,52 @@ static int __set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate)
>>   	}
>>   
>>   	scrubval = scrubrates[i].scrubval;
>> +	*scrub_bw = scrubval ? scrubrates[i].bandwidth : 0;
>>   
>> -	pci_write_bits32(ctl, SCRCTRL, scrubval, 0x001F);
>> +	return scrubval;
>> +}
>>   
>> -	if (scrubval)
>> -		return scrubrates[i].bandwidth;
>> +static inline void __set_scrub_rate(struct pci_dev *ctl, int offset,
>> +				    u32 scrubval)
>> +{
>> +	pci_write_bits32(ctl, offset, scrubval, SCRMASK);
>>   
>> -	return 0;
>>   }
>>   
> What is all that churn good for?
>
> What's wrong with simply adding the model 0x60 check to
> __set_scrub_rate() and doing the proper write there?

I was thinking it's a little better from readability POV to separate out 
the for loop which does the job of finding the scrub value to program;
And __set_scrub_rate() writes the value to the appropriate register.

Maybe I am making it too modular?

>>   
>> -	amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
>> +	if (pvt->fam == 0x15 && pvt->model == 0x60) {
>> +		/* Since we mirror the same scrubrate value across
>> +		 * both DCTs, it is enough to read the value off one of
>> +		 * the DCT registers.
>> +		 */
>> +		f15h_select_dct(pvt, 0);
> If it is enough, why do you select DCT 0? Just read the currently
> selected one, whichever it is...

Yeah, that's a good point!
Will fix this.

>>   static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
>> -				unsigned cs_mode, int cs_mask_nr)
>> +				   unsigned cs_mode, int cs_mask_nr)
>>   {
>>   	WARN_ON(cs_mode > 12);
> Why is that hunk here?
>
>> @@ -1666,7 +1699,7 @@ static int f1x_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
>>   }
>>   
>>   static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
>> -					u64 sys_addr, int *chan_sel)
>> +				       u64 sys_addr, int *chan_sel)
>>   {
>>   	int cs_found = -EINVAL;
>>   	int num_dcts_intlv = 0;
> That one too?
>

I realize it's unrelated to the patch and it's not doing something useful;
But I was thinking I'll fix the above indentation issues to keep indent 
rules consistent and since I was touching the file anyway.
Can remove those in a V2..

Thanks,
-Aravind.

Re: [PATCH 2/3] EDAC, amd64_edac: Extend scrub rate programmability feature for F15hM60h

[Borislav Petkov]

On Thu, Sep 24, 2015 at 11:15:08AM -0500, Aravind Gopalakrishnan wrote:
> I was thinking it's a little better from readability POV to separate out the
> for loop which does the job of finding the scrub value to program;
> And __set_scrub_rate() writes the value to the appropriate register.
> 
> Maybe I am making it too modular?

Yeah, you are.

The function is perfectly readable the way it is, it even fits on the
half of my screen :)

Also, it didn't really make things better - it added that *scrub_bw
argument which is a second output argument. AFAIR, it even used to be
like that at some point in time...

And that's ugly - I much prefer having input arguments being input only
and return values being return values only. If it can be helped, that
is. And in this case, it is not necessary.

> I realize it's unrelated to the patch and it's not doing something useful;
> But I was thinking I'll fix the above indentation issues to keep indent
> rules consistent and since I was touching the file anyway.
> Can remove those in a V2..

Yeah, I believe we've talked about this before: a patch should do one
logical change and one logical change *only* - no other unrelated hunks
belong in it.

Otherwise, they make reviewing it harder by making me wonder why is that
hunk there and what does it have to do with the scrub rate changes.
Unrelated hunks can - further down the road - complicate bisection and
cherry-picking for other kernels.

So please try to restrain yourself to do solely the one logical change
your patch addresses.

If you feel that some more work needs to be done on the file or the
whole driver, you can always send *separate* patches ontop which we can
discuss.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 2/3] EDAC, amd64_edac: Extend scrub rate programmability feature for F15hM60h

[Aravind Gopalakrishnan]

On 9/24/2015 11:33 AM, Borislav Petkov wrote:
>
>
> And that's ugly - I much prefer having input arguments being input only
> and return values being return values only. If it can be helped, that
> is. And in this case, it is not necessary.

Okay, I'll fix this in the next version and do a family check in 
__set_scrub_rate() itself.

>
> Yeah, I believe we've talked about this before: a patch should do one
> logical change and one logical change *only* - no other unrelated hunks
> belong in it.
>
> Otherwise, they make reviewing it harder by making me wonder why is that
> hunk there and what does it have to do with the scrub rate changes.
> Unrelated hunks can - further down the road - complicate bisection and
> cherry-picking for other kernels.
>
> So please try to restrain yourself to do solely the one logical change
> your patch addresses.
>
> If you feel that some more work needs to be done on the file or the
> whole driver, you can always send *separate* patches ontop which we can
> discuss.
>

Okay, Sorry about that.

Will clean it up in V2.

Thanks,
-Aravind.