On Wed, May 04, 2016 at 09:38:59AM -0400, Jason Baron wrote: > I've verified that the 'ce_count' is correctly incrementing with bad dimms.
Please try a bit harder when writing the commit message next time. I know it is obvious but this arbitrary sentence above hardly qualifies for a commit message and we should try to be more presentable :) > Signed-off-by: Jason Baron <[email protected]> > --- > drivers/edac/ie31200_edac.c | 110 > ++++++++++++++++++++++++++++++++------------ > 1 file changed, 81 insertions(+), 29 deletions(-) > > diff --git a/drivers/edac/ie31200_edac.c b/drivers/edac/ie31200_edac.c > index 18d77ace4813..ae77d6907892 100644 > --- a/drivers/edac/ie31200_edac.c > +++ b/drivers/edac/ie31200_edac.c > @@ -17,6 +17,7 @@ > * 015c: Xeon E3-1200 v2/3rd Gen Core processor DRAM Controller > * 0c04: Xeon E3-1200 v3/4th Gen Core Processor DRAM Controller > * 0c08: Xeon E3-1200 v3 Processor DRAM Controller > + * 1918: Xeon E3-1200 v5 Skylake Host Bridge/DRAM Registers > * > * Based on Intel specification: > * > http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e3-1200v3-vol-2-datasheet.pdf > @@ -55,6 +56,7 @@ > #define PCI_DEVICE_ID_INTEL_IE31200_HB_5 0x015c > #define PCI_DEVICE_ID_INTEL_IE31200_HB_6 0x0c04 > #define PCI_DEVICE_ID_INTEL_IE31200_HB_7 0x0c08 > +#define PCI_DEVICE_ID_INTEL_IE31200_HB_8 0x1918 > > #define IE31200_DIMMS 4 > #define IE31200_RANKS 8 > @@ -105,8 +107,11 @@ > * 1 Multiple Bit Error Status (MERRSTS) > * 0 Correctable Error Status (CERRSTS) > */ > + > #define IE31200_C0ECCERRLOG 0x40c8 > #define IE31200_C1ECCERRLOG 0x44c8 > +#define IE31200_C0ECCERRLOG_SKYLAKE 0x4048 > +#define IE31200_C1ECCERRLOG_SKYLAKE 0x4448 Please abbreviate it to "SKL" like the perf code does: arch/x86/events/intel/cstate.c:47: * Available model: NHM,WSM,SNB,IVB,HSW,BDW,SKL arch/x86/events/intel/cstate.c:51: * Available model: SLM,AMT,NHM,WSM,SNB,IVB,HSW,BDW,SKL arch/x86/events/intel/cstate.c:55: * Available model: SNB,IVB,HSW,BDW,SKL ... > #define IE31200_ECCERRLOG_CE BIT(0) > #define IE31200_ECCERRLOG_UE BIT(1) > #define IE31200_ECCERRLOG_RANK_BITS GENMASK_ULL(28, 27) > @@ -123,17 +128,28 @@ > #define IE31200_CAPID0_DDPCD BIT(6) > #define IE31200_CAPID0_ECC BIT(1) > > -#define IE31200_MAD_DIMM_0_OFFSET 0x5004 > -#define IE31200_MAD_DIMM_SIZE GENMASK_ULL(7, 0) > -#define IE31200_MAD_DIMM_A_RANK BIT(17) > -#define IE31200_MAD_DIMM_A_WIDTH BIT(19) > - > -#define IE31200_PAGES(n) (n << (28 - PAGE_SHIFT)) > +#define IE31200_MAD_DIMM_0_OFFSET 0x5004 > +#define IE31200_MAD_DIMM_0_OFFSET_SKYLAKE 0x500C > +#define IE31200_MAD_DIMM_SIZE GENMASK_ULL(7, 0) > +#define IE31200_MAD_DIMM_A_RANK BIT(17) > +#define IE31200_MAD_DIMM_A_RANK_SHIFT 17 > +#define IE31200_MAD_DIMM_A_RANK_SKYLAKE BIT(10) > +#define IE31200_MAD_DIMM_A_RANK_SKYLAKE_SHIFT 10 > +#define IE31200_MAD_DIMM_A_WIDTH BIT(19) > +#define IE31200_MAD_DIMM_A_WIDTH_SHIFT 19 > +#define IE31200_MAD_DIMM_A_WIDTH_SKYLAKE GENMASK_ULL(9, 8) > +#define IE31200_MAD_DIMM_A_WIDTH_SKYLAKE_SHIFT 8 > + > +/* skylake reports 1GB increments/everything else is 256MB */ s/skylake/Skylake/g > +#define IE31200_PAGES(n, sky) \ > + (n << (28 + (2 * sky) - PAGE_SHIFT)) > > static int nr_channels; > > struct ie31200_priv { > void __iomem *window; > + void __iomem *c0errlog; > + void __iomem *c1errlog; > }; > > enum ie31200_chips { ... > @@ -363,7 +379,10 @@ static int ie31200_probe1(struct pci_dev *pdev, int > dev_idx) > > edac_dbg(3, "MC: init mci\n"); > mci->pdev = &pdev->dev; > - mci->mtype_cap = MEM_FLAG_DDR3; > + if (skylake) > + mci->mtype_cap = MEM_FLAG_DDR4; > + else > + mci->mtype_cap = MEM_FLAG_DDR3; > mci->edac_ctl_cap = EDAC_FLAG_SECDED; > mci->edac_cap = EDAC_FLAG_SECDED; > mci->mod_name = EDAC_MOD_STR; > @@ -374,19 +393,43 @@ static int ie31200_probe1(struct pci_dev *pdev, int > dev_idx) > mci->ctl_page_to_phys = NULL; > priv = mci->pvt_info; > priv->window = window; > + if (skylake) { > + priv->c0errlog = window + IE31200_C0ECCERRLOG_SKYLAKE; > + priv->c1errlog = window + IE31200_C1ECCERRLOG_SKYLAKE; > + mad_offset = IE31200_MAD_DIMM_0_OFFSET_SKYLAKE; > + } else { > + priv->c0errlog = window + IE31200_C0ECCERRLOG; > + priv->c1errlog = window + IE31200_C1ECCERRLOG; > + mad_offset = IE31200_MAD_DIMM_0_OFFSET; > + } > > /* populate DIMM info */ > for (i = 0; i < IE31200_CHANNELS; i++) { > - addr_decode = readl(window + IE31200_MAD_DIMM_0_OFFSET + > + addr_decode = readl(window + mad_offset + > (i * 4)); > edac_dbg(0, "addr_decode: 0x%x\n", addr_decode); > for (j = 0; j < IE31200_DIMMS_PER_CHANNEL; j++) { > - dimm_info[i][j].size = (addr_decode >> (j * 8)) & > - IE31200_MAD_DIMM_SIZE; > - dimm_info[i][j].dual_rank = (addr_decode & > - (IE31200_MAD_DIMM_A_RANK << j)) ? 1 : 0; > - dimm_info[i][j].x16_width = (addr_decode & > - (IE31200_MAD_DIMM_A_WIDTH << j)) ? 1 : 0; > + if (skylake) { > + dimm_info[i][j].size = (addr_decode >> > + (j * 16)) & IE31200_MAD_DIMM_SIZE; > + dimm_info[i][j].dual_rank = ((addr_decode & > + (IE31200_MAD_DIMM_A_RANK_SKYLAKE << > + (j * 16))) >> > + (IE31200_MAD_DIMM_A_RANK_SKYLAKE_SHIFT + > + (j * 16))); > + dimm_info[i][j].x16_width = ((addr_decode & > + (IE31200_MAD_DIMM_A_WIDTH_SKYLAKE << > + (j * 16))) >> > + (IE31200_MAD_DIMM_A_WIDTH_SKYLAKE_SHIFT > + + (j * 16))); So these lines were unreadable before and now with the additional indentation level they're even worse. I think you should let them stick out. Better yet, they're screaming to be carved out into separate functions; something like: * populate_dimm_info() which is called by ie31200_probe1() * in populate_dimm_info() you check whether you're on an SKL part and then do: if (skylake) __skl_populate_dimm_info() else __old_populate_dimm_info() and this way you can save yourself the sprinkling of "if (skylake)" a but because imagine what that code is going to look when you add support for a third model. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply.
