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

[Borislav Petkov <bp@alien8.de>]

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.