Re: [PATCH V2] edac, amd64_edac: Modify usage of amd64_read_dct_pci_cfg()

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

On 9/2/2014 2:08 AM, Borislav Petkov wrote:
> On Tue, Aug 26, 2014 at 12:44:09PM -0500, Aravind Gopalakrishnan wrote:
>> Rationale behind this change:
>>   - F2x1xx addresses were stopped from being mapped explicitly to DCT1
>>     from F15h (OR) onwards. They use _dct[0:1] mechanism to access the
>>     registers. So we should move away from using address ranges to select
>>     DCT for these families.
>>   - On newer processors, the address ranges used to indicate DCT1 (0x140,
>>     0x1a0) have different meanings than what is assumed currently.
>>
>> Changes introduced:
>>   - amd64_read_dct_pci_cfg() now takes in dct value and uses it for
>>     'selecting the dct'
>>   - Update usage of the function
>>   - Remove [k8|f10]_read_dct_pci_cfg() as they don't do much different
>>     from amd64_read_pci_cfg().
>>     - Move the k8 specific check to amd64_read_pci_cfg
>>   - Remove now needless .read_dct_pci_cfg
>>
>> Testing:
>>   - Tested on Fam 10h; Fam15h Models: 00h, 30h; Fam16h using 'EDAC_DEBUG'
>>     and mce_amd_inj
>>   - driver obtains info from F2x registers and caches it in pvt
>>     structures correctly
>>   - ECC decoding wotks fine
>>
>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>> ---
>> Changes in V2: (per Boris suggestion)
>>   - Hide family checks in amd64_read_dct_pci_cfg()
>>   - Use pvt structure for family checks and not boot_cpu_data
>>
>>   drivers/edac/amd64_edac.c | 134 ++++++++++++++++++++++------------------------
>>   drivers/edac/amd64_edac.h |  23 ++++++--
>>   2 files changed, 83 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
>> index f8bf000..ba0b78e 100644
>> --- a/drivers/edac/amd64_edac.c
>> +++ b/drivers/edac/amd64_edac.c
>> @@ -60,6 +60,20 @@ static const struct scrubrate {
>>   	{ 0x00, 0UL},        /* scrubbing off */
>>   };
>>   
>>
>>   
>> @@ -767,17 +747,21 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
>>   		int reg1   = DCSB1 + (cs * 4);
>>   		u32 *base0 = &pvt->csels[0].csbases[cs];
>>   		u32 *base1 = &pvt->csels[1].csbases[cs];
>> +		u8 dct = 0;
>>   
>> -		if (!amd64_read_dct_pci_cfg(pvt, reg0, base0))
>> +		if (!amd64_read_dct_pci_cfg(pvt, dct, reg0, base0))
>>   			edac_dbg(0, "  DCSB0[%d]=0x%08x reg: F2x%x\n",
>>   				 cs, *base0, reg0);
>>   
>> -		if (pvt->fam == 0xf || dct_ganging_enabled(pvt))
>> +		if (pvt->fam == 0xf)
>>   			continue;
>>   
>> -		if (!amd64_read_dct_pci_cfg(pvt, reg1, base1))
>> +		dct = ((pvt->fam == 0x15)
>> +			&& (pvt->model == 0x30)) ? 3 : 1;
> That selection can go into amd64_read_dct_pci_cfg() too, right?

I still need to pass '0' or '1' for the dct value. But sure, shall move 
the check to amd64_read_dct_pci_cfg()

>>   
>> -	if (!dct_ganging_enabled(pvt)) {
>> -		amd64_read_dct_pci_cfg(pvt, DCLR1, &pvt->dclr1);
>> -		amd64_read_dct_pci_cfg(pvt, DCHR1, &pvt->dchr1);
>> -	}
>> +	dct = ((pvt->fam == 0x15)
>> +		&& (pvt->model == 0x30)) ? 3 : 1;
>> +	amd64_read_dct_pci_cfg(pvt, dct, DCLR0, &pvt->dclr1);
>> +	amd64_read_dct_pci_cfg(pvt, dct, DCHR0, &pvt->dchr1);
>>   
>>   	pvt->ecc_sym_sz = 4;
>>   
>>   	if (pvt->fam >= 0x10) {
>>   		amd64_read_pci_cfg(pvt->F3, EXT_NB_MCA_CFG, &tmp);
>> -		if (pvt->fam != 0x16)
>> -			/* F16h has only DCT0 */
>> -			amd64_read_dct_pci_cfg(pvt, DBAM1, &pvt->dbam1);
>> +		if (pvt->fam == 0x10)
>> +			amd64_read_pci_cfg(pvt->F2, DBAM1, &pvt->dbam1);
>> +		/* F16h has only DCT0, so no need to read dbam1 */
>> +		else if (pvt->fam == 0x15) {
> This logic can be moved into amd64_read_dct_pci_cfg() too?

Fam10h needs some careful handling since I see that in some places I do 
amd64_read_dct_pci_cfg()
dct_ganging need to be disabled and in some places need not be. (This is 
why I had the condition above)
I think I can work around this..
Let me give it a shot and send you a V3.

> I think you can get rid of the f15_read_dct_pci_cfg() completely and
> move the logic into amd64_read_dct_pci_cfg()
>

Ok.

Thanks,
-Aravind.