Re: [PATCH] ie31200_edac: add skylake support

From: Borislav Petkov
Date: Fri May 06 2016 - 04:02:54 EST

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 <jbaron@xxxxxxxxxx>
> ---
> 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.