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

[Aravind Gopalakrishnan]

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@xxxxxxx>
> > ---
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/