Date:	Tue, 8 Apr 2014 11:09:24 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Jason Baron <jbaron@...mai.com>
Cc:	tony.luck@...el.com, hpa@...or.com, mingo@...nel.org,
	dougthompson@...ssion.com, m.chehab@...sung.com,
	mitake@....info.waseda.ac.jp, linux-edac@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] ie31200_edac: Add driver

On Fri, Apr 04, 2014 at 09:14:04PM +0000, Jason Baron wrote:
> Add 'ie31200_edac' driver for the E3-1200 series of Intel processors. Driver
> is based on the following E3-1200 specs:
> 
> http://www.intel.com/content/www/us/en/processors/xeon/xeon-e3-1200-family-vol-2-datasheet.html
> http://www.intel.com/content/www/us/en/processors/xeon/xeon-e3-1200v3-vol-2-datasheet.html
> 
> I've tested this on bad memory hardware, and observed correlating bad reads
> and uncorrected memory errors as reported by the driver.
> 
> Signed-off-by: Jason Baron <jbaron@...mai.com>
> ---
>  drivers/edac/Kconfig        |   7 +
>  drivers/edac/Makefile       |   1 +
>  drivers/edac/ie31200_edac.c | 540 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 548 insertions(+)
>  create mode 100644 drivers/edac/ie31200_edac.c
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 878f090..27f44a1 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -186,6 +186,13 @@ config EDAC_I3200
>  	  Support for error detection and correction on the Intel
>  	  3200 and 3210 server chipsets.
>  
> +config EDAC_IE31200
> +	tristate "Intel e312xx"
> +	depends on EDAC_MM_EDAC && PCI && X86
> +	help
> +	  Support for error detection and correction on the Intel
> +	  E3-1200 processor.
> +
>  config EDAC_X38
>  	tristate "Intel X38"
>  	depends on EDAC_MM_EDAC && PCI && X86
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index 4154ed6..c479a24 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_EDAC_I82875P)		+= i82875p_edac.o
>  obj-$(CONFIG_EDAC_I82975X)		+= i82975x_edac.o
>  obj-$(CONFIG_EDAC_I3000)		+= i3000_edac.o
>  obj-$(CONFIG_EDAC_I3200)		+= i3200_edac.o
> +obj-$(CONFIG_EDAC_IE31200)		+= ie31200_edac.o
>  obj-$(CONFIG_EDAC_X38)			+= x38_edac.o
>  obj-$(CONFIG_EDAC_I82860)		+= i82860_edac.o
>  obj-$(CONFIG_EDAC_R82600)		+= r82600_edac.o
> diff --git a/drivers/edac/ie31200_edac.c b/drivers/edac/ie31200_edac.c
> new file mode 100644
> index 0000000..ae03d21
> --- /dev/null
> +++ b/drivers/edac/ie31200_edac.c
> @@ -0,0 +1,540 @@
> +/*
> + * Intel E3-1200
> + * Copyright (C) 2014 Jason Baron <jbaron@...mai.com>
> + *
> + * Support for the E3-1200 processor family. Heavily based on previous
> + * Intel EDAC drivers.
> + *
> + * Since the DRAM controller is on the cpu chip, we can use its PCI device
> + * id to identify these processors.
> + *
> + * PCI DRAM controller device ids (Taken from The PCI ID Repository - http://pci-ids.ucw.cz/)
> + *
> + * 0108: Xeon E3-1200 Processor Family DRAM Controller
> + * 010c: Xeon E3-1200/2nd Generation Core Processor Family DRAM Controller
> + * 0150: Xeon E3-1200 v2/3rd Gen Core processor DRAM Controller
> + * 0158: Xeon E3-1200 v2/Ivy Bridge DRAM Controller
> + * 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
> + *
> + * Based on Intel specification:
> + * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e3-1200v3-vol-2-datasheet.pdf
> + * http://www.intel.com/content/www/us/en/processors/xeon/xeon-e3-1200-family-vol-2-datasheet.html
> + *
> + * According to the above datasheet (p.16):
> + * "
> + * 6. Software must not access B0/D0/F0 32-bit memory-mapped registers with
> + * requests that cross a DW boundary.
> + * "
> + *
> + * Thus, we make use of the explicit: lo_hi_readq(), which breaks the readq into
> + * 2 readl() calls. This restriction may be lifted in subsequent chip releases,
> + * but lo_hi_readq() ensures that we are safe across all e3-1200 processors.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <linux/pci_ids.h>
> +#include <linux/edac.h>
> +
> +#include <asm-generic/io-64-nonatomic-lo-hi.h>
> +#include "edac_core.h"
> +
> +#define IE31200_REVISION "1.0"
> +#define EDAC_MOD_STR "ie31200_edac"
> +
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_1 0x0108
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_2 0x010c
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_3 0x0150
> +#define PCI_DEVICE_ID_INTEL_IE31200_HB_4 0x0158
> +#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 IE31200_DIMMS			4
> +#define IE31200_RANKS			8
> +#define IE31200_RANKS_PER_CHANNEL	4
> +#define IE31200_DIMMS_PER_CHANNEL	2
> +#define IE31200_CHANNELS		2
> +
> +/* Intel IE31200 register addresses - device 0 function 0 - DRAM Controller */
> +#define IE31200_MCHBAR_LOW	0x48
> +#define IE31200_MCHBAR_HIGH	0x4c
> +#define IE31200_MCHBAR_MASK	0xffffff8000ULL	/* bits 38:15 */

GENMASK_ULL()

> +#define IE31200_MMR_WINDOW_SIZE	32768

(1 << 15) ?

> +
> +/* Error Status Register (16b)
> + *
> + * 15    reserved
> + * 14    Isochronous TBWRR Run Behind FIFO Full
> + *       (ITCV)
> + * 13    Isochronous TBWRR Run Behind FIFO Put
> + *       (ITSTV)
> + * 12    reserved
> + * 11    MCH Thermal Sensor Event
> + *       for SMI/SCI/SERR (GTSE)
> + * 10    reserved
> + *  9    LOCK to non-DRAM Memory Flag (LCKF)
> + *  8    reserved
> + *  7    DRAM Throttle Flag (DTF)
> + *  6:2  reserved
> + *  1    Multi-bit DRAM ECC Error Flag (DMERR)
> + *  0    Single-bit DRAM ECC Error Flag (DSERR)
> + */
> +#define IE31200_ERRSTS		0xc8
> +#define IE31200_ERRSTS_UE	0x0002

Let's use the BIT() macro for those:

BIT(1)

> +#define IE31200_ERRSTS_CE	0x0001

BIT(0)

> +#define IE31200_ERRSTS_BITS	(IE31200_ERRSTS_UE | IE31200_ERRSTS_CE)
> +
> +/*
> + * Channel 0 ECC Error Log (64b)
> + *
> + * 63:48 Error Column Address (ERRCOL)
> + * 47:32 Error Row Address (ERRROW)
> + * 31:29 Error Bank Address (ERRBANK)
> + * 28:27 Error Rank Address (ERRRANK)
> + * 26:24 reserved
> + * 23:16 Error Syndrome (ERRSYND)
> + * 15: 2 reserved
> + *    1  Multiple Bit Error Status (MERRSTS)
> + *    0  Correctable Error Status (CERRSTS)
> + */
> +#define IE31200_C0ECCERRLOG			0x40c8
> +#define IE31200_C1ECCERRLOG			0x44c8
> +#define IE31200_ECCERRLOG_CE			0x1
> +#define IE31200_ECCERRLOG_UE			0x2

Ditto.

> +#define IE31200_ECCERRLOG_RANK_BITS		0x18000000

GENMASK

> +#define IE31200_ECCERRLOG_RANK_SHIFT		27
> +#define IE31200_ECCERRLOG_SYNDROME_BITS		0xff0000

Ditto.

> +#define IE31200_ECCERRLOG_SYNDROME_SHIFT	16
> +#define IE31200_CAPID0				0xe4
> +
> +#define IE31200_MAD_DIMM_0_OFFSET 0x5004
> +
> +#define IE31200_PAGES(n) \
> +	(n << (28 - PAGE_SHIFT))
> +
> +struct ie31200_priv {
> +	void __iomem *window;
> +};
> +
> +static int nr_channels;
> +
> +static int how_many_channels(struct pci_dev *pdev)
> +{
> +	int n_channels;
> +	unsigned char capid0_2b; /* 2nd byte of CAPID0 */
> +
> +	pci_read_config_byte(pdev, IE31200_CAPID0 + 1, &capid0_2b);
> +
> +	/* check PDCD: Dual Channel Disable */
> +	if (capid0_2b & 0x10) {

Now that you have nice defines for all those bits, why not do that for
those naked numbers too? :-)

Maybe even local to this function:

#define PDCD	BIT(4)


> +		edac_dbg(0, "In single channel mode\n");
> +		n_channels = 1;
> +	} else {
> +		edac_dbg(0, "In dual channel mode\n");
> +		n_channels = 2;
> +	}
> +
> +	/* check DDPCD - check if both channels are filled */

Ditto:

#define DDPCD BIT(6)

> +	if (capid0_2b & 0x40)
> +		edac_dbg(0, "2 DIMMS per channel disabled\n");
> +	else
> +		edac_dbg(0, "2 DIMMS per channel enabled\n");
> +
> +	return n_channels;
> +}
> +
> +static bool ecc_capable(struct pci_dev *pdev)
> +{
> +	unsigned char capid0_4b; /* 4th byte of CAPID0 */
> +
> +	pci_read_config_byte(pdev, IE31200_CAPID0 + 3, &capid0_4b);
> +	if (capid0_4b & 0x2)

BIT(1) at least, if no define.

> +		return false;
> +	return true;
> +}
> +
> +static unsigned long eccerrlog_syndrome(u64 log)
> +{
> +	return (log & IE31200_ECCERRLOG_SYNDROME_BITS) >>
> +		IE31200_ECCERRLOG_SYNDROME_SHIFT;
> +}

Looks like this wants to be a macro.

> +
> +static int eccerrlog_row(int channel, u64 log)
> +{
> +	u64 rank = ((log & IE31200_ECCERRLOG_RANK_BITS) >>
> +		IE31200_ECCERRLOG_RANK_SHIFT);
> +	return rank;
> +}

Ditto.

> +
> +enum ie31200_chips {
> +	IE31200 = 0,
> +};
> +
> +struct ie31200_dev_info {
> +	const char *ctl_name;
> +};
> +
> +struct ie31200_error_info {
> +	u16 errsts;
> +	u16 errsts2;
> +	u64 eccerrlog[IE31200_CHANNELS];
> +};
> +
> +static const struct ie31200_dev_info ie31200_devs[] = {
> +	[IE31200] = {
> +		.ctl_name = "IE31200"
> +	},
> +};

Maybe pull those up before the first function definition so that you can
have all struct definitions together.

> +
> +static void ie31200_clear_error_info(struct mem_ctl_info *mci)
> +{
> +	struct pci_dev *pdev;
> +
> +	pdev = to_pci_dev(mci->pdev);

Just drop all the local gaming by simply doing:

	pci_write_bits16(to_pci_dev(mci->pdev), IE31200_ERRSTS,
			 IE31200_ERRSTS_BITS,
			 IE31200_ERRSTS_BITS);

> +
> +	/*
> +	 * Clear any error bits.
> +	 * (Yes, we really clear bits by writing 1 to them.)

Yeah, that's that write-to-clear mechanism.

> +	 */
> +	pci_write_bits16(pdev, IE31200_ERRSTS, IE31200_ERRSTS_BITS,
> +		IE31200_ERRSTS_BITS);

Arg alignment.

> +}
> +
> +static void ie31200_get_and_clear_error_info(struct mem_ctl_info *mci,
> +		struct ie31200_error_info *info)

Ditto.

> +{
> +	struct pci_dev *pdev;
> +	struct ie31200_priv *priv = mci->pvt_info;
> +	void __iomem *window = priv->window;
> +
> +	pdev = to_pci_dev(mci->pdev);
> +
> +	/*
> +	 * This is a mess because there is no atomic way to read all the
> +	 * registers at once and the registers can transition from CE being
> +	 * overwritten by UE.
> +	 */

For the UE you'd get an MCE, right?

Btw, this driver is polling, AFAICT. Doesn't e3-12xx support the CMCI
interrupt which you can feed into this driver directly and thus not need
the polling at all?

> +	pci_read_config_word(pdev, IE31200_ERRSTS, &info->errsts);
> +	if (!(info->errsts & IE31200_ERRSTS_BITS))
> +		return;
> +
> +	info->eccerrlog[0] = lo_hi_readq(window + IE31200_C0ECCERRLOG);
> +	if (nr_channels == 2)
> +		info->eccerrlog[1] = lo_hi_readq(window + IE31200_C1ECCERRLOG);
> +
> +	pci_read_config_word(pdev, IE31200_ERRSTS, &info->errsts2);
> +
> +	/*
> +	 * If the error is the same for both reads then the first set
> +	 * of reads is valid.  If there is a change then there is a CE
> +	 * with no info and the second set of reads is valid and
> +	 * should be UE info.
> +	 */
> +	if ((info->errsts ^ info->errsts2) & IE31200_ERRSTS_BITS) {
> +		info->eccerrlog[0] = lo_hi_readq(window + IE31200_C0ECCERRLOG);
> +		if (nr_channels == 2)
> +			info->eccerrlog[1] =
> +				lo_hi_readq(window + IE31200_C1ECCERRLOG);
> +	}
> +
> +	ie31200_clear_error_info(mci);
> +}
> +
> +static void ie31200_process_error_info(struct mem_ctl_info *mci,
> +		struct ie31200_error_info *info)

arg alignment.

> +{
> +	int channel;
> +	u64 log;
> +
> +	if (!(info->errsts & IE31200_ERRSTS_BITS))
> +		return;
> +
> +	if ((info->errsts ^ info->errsts2) & IE31200_ERRSTS_BITS) {
> +		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1, 0, 0, 0,
> +				     -1, -1, -1, "UE overwrote CE", "");
> +		info->errsts = info->errsts2;
> +	}
> +
> +	for (channel = 0; channel < nr_channels; channel++) {
> +		log = info->eccerrlog[channel];
> +		if (log & IE31200_ECCERRLOG_UE) {
> +			edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
> +					     0, 0, 0,
> +					     eccerrlog_row(channel, log),
> +					     channel, -1,
> +					     "i3000 UE", "");
> +		} else if (log & IE31200_ECCERRLOG_CE) {
> +			edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1,
> +					     0, 0, eccerrlog_syndrome(log),
> +					     eccerrlog_row(channel, log),
> +					     channel, -1,
> +					     "i3000 CE", "");
> +		}
> +	}
> +}
> +
> +static void ie31200_check(struct mem_ctl_info *mci)
> +{
> +	struct ie31200_error_info info;
> +
> +	edac_dbg(1, "MC%d\n", mci->mc_idx);
> +	ie31200_get_and_clear_error_info(mci, &info);
> +	ie31200_process_error_info(mci, &info);
> +}
> +
> +static void __iomem *ie31200_map_mchbar(struct pci_dev *pdev)
> +{
> +	union {
> +		u64 mchbar;
> +		struct {
> +			u32 mchbar_low;
> +			u32 mchbar_high;
> +		};
> +	} u;
> +	void __iomem *window;
> +
> +	pci_read_config_dword(pdev, IE31200_MCHBAR_LOW, &u.mchbar_low);
> +	pci_read_config_dword(pdev, IE31200_MCHBAR_HIGH, &u.mchbar_high);
> +	u.mchbar &= IE31200_MCHBAR_MASK;
> +
> +	if (u.mchbar != (resource_size_t)u.mchbar) {
> +		pr_err("ie31200: mmio space beyond accessible range (0x%llx)\n",
> +			(unsigned long long)u.mchbar);

We have edac_*_printk for those, which takes care of the prefix, etc.

> +		return NULL;
> +	}
> +
> +	window = ioremap_nocache(u.mchbar, IE31200_MMR_WINDOW_SIZE);
> +	if (!window)
> +		pr_err("ie31200: cannot map mmio space at 0x%llx\n",
> +			(unsigned long long)u.mchbar);

Ditto.

> +
> +	return window;
> +}
> +
> +struct dimm_data {
> +	u8 size; /* in 256MB multiples */
> +	u8 dual_rank : 1,
> +	   x16_width : 1; /* 0 means x8 width */
> +};

Also move up to the rest of the struct definitions.

> +static int ie31200_probe1(struct pci_dev *pdev, int dev_idx)
> +{
> +	int rc;
> +	int i, j;
> +	struct mem_ctl_info *mci = NULL;
> +	struct edac_mc_layer layers[2];
> +	struct dimm_data dimm_info[IE31200_CHANNELS][IE31200_DIMMS_PER_CHANNEL];
> +	void __iomem *window;
> +	struct ie31200_priv *priv;
> +	u32 addr_decode;
> +
> +	edac_dbg(0, "MC:\n");
> +
> +	if (!ecc_capable(pdev)) {
> +		edac_dbg(1, "Not edac capable\n");

dbg? Why not info? Also, "No ECC support" or somesuch, "edac capable" is funny :-)

> +		return -ENODEV;
> +	}
> +
> +	window = ie31200_map_mchbar(pdev);
> +	if (!window)
> +		return -ENODEV;
> +
> +	/* populate DIMM info */
> +	for (i = 0; i < IE31200_CHANNELS; i++) {
> +		addr_decode = readl(window + IE31200_MAD_DIMM_0_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)) & 0xff;
> +			dimm_info[i][j].dual_rank =
> +				(addr_decode & (0x20000 << j)) ? 1 : 0;
> +			dimm_info[i][j].x16_width =
> +				(addr_decode & (0x80000 << j)) ? 1 : 0;

#define's for those naked bits?

> +			edac_dbg(0, "size: 0x%x, rank: %d, width: %d\n",
> +				 dimm_info[i][j].size,
> +				 dimm_info[i][j].dual_rank,
> +				 dimm_info[i][j].x16_width);
> +		}
> +	}
> +
> +	nr_channels = how_many_channels(pdev);
> +
> +	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> +	layers[0].size = IE31200_DIMMS;
> +	layers[0].is_virt_csrow = true;
> +	layers[1].type = EDAC_MC_LAYER_CHANNEL;
> +	layers[1].size = nr_channels;
> +	layers[1].is_virt_csrow = false;
> +	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> +			    sizeof(struct ie31200_priv));
> +
> +	rc = -ENOMEM;
> +	if (!mci)
> +		goto fail_unmap;
> +
> +	edac_dbg(3, "MC: init mci\n");
> +
> +	mci->pdev = &pdev->dev;
> +	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;
> +	mci->mod_ver = IE31200_REVISION;
> +	mci->ctl_name = ie31200_devs[dev_idx].ctl_name;
> +	mci->dev_name = pci_name(pdev);
> +	mci->edac_check = ie31200_check;
> +	mci->ctl_page_to_phys = NULL;
> +	priv = mci->pvt_info;
> +	priv->window = window;
> +
> +	/*
> +	 * The dram rank boundary (DRB) reg values are boundary addresses
> +	 * for each DRAM rank with a granularity of 64MB.  DRB regs are
> +	 * cumulative; the last one will contain the total memory
> +	 * contained in all ranks.
> +	 */
> +	for (i = 0; i < IE31200_DIMMS_PER_CHANNEL; i++) {
> +		for (j = 0; j < IE31200_CHANNELS; j++) {
> +			struct dimm_info *dimm;
> +			unsigned long nr_pages;
> +
> +			nr_pages = IE31200_PAGES(dimm_info[j][i].size);
> +			if (nr_pages == 0)
> +				continue;
> +
> +			if (dimm_info[j][i].dual_rank) {
> +				nr_pages = nr_pages / 2;
> +				dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
> +						     mci->n_layers, (i * 2) + 1,
> +						     j, 0);
> +				dimm->nr_pages = nr_pages;
> +				edac_dbg(0, "set nr pages: 0x%lx\n", nr_pages);
> +				dimm->grain = 8; /* just a guess */
> +				dimm->mtype = MEM_DDR3;
> +				dimm->dtype = DEV_UNKNOWN;
> +				dimm->edac_mode = EDAC_UNKNOWN;
> +			}
> +			dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
> +					     mci->n_layers, i * 2, j, 0);
> +			dimm->nr_pages = nr_pages;
> +			edac_dbg(0, "set nr pages: 0x%lx\n", nr_pages);
> +			dimm->grain = 8; /* same guess */
> +			dimm->mtype = MEM_DDR3;
> +			dimm->dtype = DEV_UNKNOWN;
> +			dimm->edac_mode = EDAC_UNKNOWN;
> +		}
> +	}
> +
> +	ie31200_clear_error_info(mci);
> +
> +	rc = -ENODEV;
> +	if (edac_mc_add_mc(mci)) {
> +		edac_dbg(3, "MC: failed edac_mc_add_mc()\n");
> +		goto fail_free;
> +	}
> +
> +	/* get this far and it's successful */
> +	edac_dbg(3, "MC: success\n");
> +	return 0;
> +
> +fail_free:
> +	if (mci)
> +		edac_mc_free(mci);
> +fail_unmap:
> +	iounmap(window);
> +
> +	return rc;
> +}
> +
> +static int ie31200_init_one(struct pci_dev *pdev,
> +			    const struct pci_device_id *ent)
> +{
> +	int rc;
> +
> +	edac_dbg(0, "MC:\n");
> +
> +	if (pci_enable_device(pdev) < 0)
> +		return -EIO;
> +
> +	rc = ie31200_probe1(pdev, ent->driver_data);

return ie31200_probe1(..)

and kill rc.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists