linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] EDAC, ie31200: Add Intel Coffee Lake CPU support
@ 2019-06-09 15:16 Marco Elver
  2019-06-10 18:01 ` Luck, Tony
  0 siblings, 1 reply; 4+ messages in thread
From: Marco Elver @ 2019-06-09 15:16 UTC (permalink / raw)
  To: jbaron
  Cc: linux-kernel, Marco Elver, Borislav Petkov,
	Mauro Carvalho Chehab, Tony Luck, linux-edac

Coffee Lake seems to work like Skylake and Kaby Lake. This patch adds all
device IDs for Coffee Lake-S CPUs according to datasheet.

Cc: Jason Baron <jbaron@akamai.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: linux-edac <linux-edac@vger.kernel.org>
---

Tested with a Xeon E-2124G.
---
 drivers/edac/ie31200_edac.c | 95 +++++++++++++++++++++++++------------
 1 file changed, 64 insertions(+), 31 deletions(-)

diff --git a/drivers/edac/ie31200_edac.c b/drivers/edac/ie31200_edac.c
index adf60eb45bd4..e9ee3ff608b2 100644
--- a/drivers/edac/ie31200_edac.c
+++ b/drivers/edac/ie31200_edac.c
@@ -20,11 +20,13 @@
  * 0c08: Xeon E3-1200 v3 Processor DRAM Controller
  * 1918: Xeon E3-1200 v5 Skylake Host Bridge/DRAM Registers
  * 5918: Xeon E3-1200 Xeon E3-1200 v6/7th Gen Core Processor Host Bridge/DRAM Registers
+ * 3e..: 8th/9th Gen Core Processor 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
  * 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/core/7th-gen-core-family-mobile-h-processor-lines-datasheet-vol-2.html
+ * https://www.intel.com/content/www/us/en/products/docs/processors/core/8th-gen-core-family-datasheet-vol-2.html
  *
  * According to the above datasheet (p.16):
  * "
@@ -61,6 +63,26 @@
 #define PCI_DEVICE_ID_INTEL_IE31200_HB_8 0x1918
 #define PCI_DEVICE_ID_INTEL_IE31200_HB_9 0x5918
 
+/* Coffee Lake-S */
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_MASK 0x3e00
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_1    0x3e0f
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_2    0x3e18
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_3    0x3e1f
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_4    0x3e30
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_5    0x3e31
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_6    0x3e32
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_7    0x3e33
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_8    0x3ec2
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_9    0x3ec6
+#define PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_10   0x3eca
+
+/* Helper macro to test if HB is for Skylake or later. */
+#define DEVICE_ID_SKYLAKE_OR_LATER(did)                                        \
+	(((did) == PCI_DEVICE_ID_INTEL_IE31200_HB_8) ||                        \
+	 ((did) == PCI_DEVICE_ID_INTEL_IE31200_HB_9) ||                        \
+	 (((did)&PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_MASK) ==                   \
+	  PCI_DEVICE_ID_INTEL_IE31200_HB_CFL_MASK))
+
 #define IE31200_DIMMS			4
 #define IE31200_RANKS			8
 #define IE31200_RANKS_PER_CHANNEL	4
@@ -381,10 +403,10 @@ static int ie31200_probe1(struct pci_dev *pdev, int dev_idx)
 	u32 addr_decode, mad_offset;
 
 	/*
-	 * Kaby Lake seems to work like Skylake. Please re-visit this logic
-	 * when adding new CPU support.
+	 * Kaby Lake, Coffee Lake seem to work like Skylake. Please re-visit
+	 * this logic when adding new CPU support.
 	 */
-	bool skl = (pdev->device >= PCI_DEVICE_ID_INTEL_IE31200_HB_8);
+	bool skl = DEVICE_ID_SKYLAKE_OR_LATER(pdev->device);
 
 	edac_dbg(0, "MC:\n");
 
@@ -542,36 +564,47 @@ static void ie31200_remove_one(struct pci_dev *pdev)
 }
 
 static const struct pci_device_id ie31200_pci_tbl[] = {
-	{
-		PCI_VEND_DEV(INTEL, IE31200_HB_1), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		IE31200},
-	{
-		PCI_VEND_DEV(INTEL, IE31200_HB_2), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		IE31200},
-	{
-		PCI_VEND_DEV(INTEL, IE31200_HB_3), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		IE31200},
-	{
-		PCI_VEND_DEV(INTEL, IE31200_HB_4), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		IE31200},
-	{
-		PCI_VEND_DEV(INTEL, IE31200_HB_5), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		IE31200},
-	{
-		PCI_VEND_DEV(INTEL, IE31200_HB_6), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		IE31200},
-	{
-		PCI_VEND_DEV(INTEL, IE31200_HB_7), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		IE31200},
-	{
-		PCI_VEND_DEV(INTEL, IE31200_HB_8), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		IE31200},
-	{
-		PCI_VEND_DEV(INTEL, IE31200_HB_9), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		IE31200},
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_1), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_2), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_3), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_4), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_5), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_6), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_7), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_8), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_9), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_1), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_2), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_3), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_4), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_5), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_6), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_7), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_8), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_9), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
+	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_10), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  IE31200 },
 	{
 		0,
-	}            /* 0 terminated list. */
+	} /* 0 terminated list. */
 };
 MODULE_DEVICE_TABLE(pci, ie31200_pci_tbl);
 
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] EDAC, ie31200: Add Intel Coffee Lake CPU support
  2019-06-09 15:16 [PATCH] EDAC, ie31200: Add Intel Coffee Lake CPU support Marco Elver
@ 2019-06-10 18:01 ` Luck, Tony
  2019-06-10 18:37   ` Marco Elver
  0 siblings, 1 reply; 4+ messages in thread
From: Luck, Tony @ 2019-06-10 18:01 UTC (permalink / raw)
  To: Marco Elver
  Cc: jbaron, linux-kernel, Borislav Petkov, Mauro Carvalho Chehab, linux-edac

On Sun, Jun 09, 2019 at 05:16:13PM +0200, Marco Elver wrote:

Marco,

Thanks for the patch. One comment below.

> -	{
> -		PCI_VEND_DEV(INTEL, IE31200_HB_1), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> -		IE31200},
> -	{
> -		PCI_VEND_DEV(INTEL, IE31200_HB_2), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> -		IE31200},
> -	{
> -		PCI_VEND_DEV(INTEL, IE31200_HB_3), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> -		IE31200},
> -	{
> -		PCI_VEND_DEV(INTEL, IE31200_HB_4), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> -		IE31200},
> -	{
> -		PCI_VEND_DEV(INTEL, IE31200_HB_5), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> -		IE31200},
> -	{
> -		PCI_VEND_DEV(INTEL, IE31200_HB_6), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> -		IE31200},
> -	{
> -		PCI_VEND_DEV(INTEL, IE31200_HB_7), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> -		IE31200},
> -	{
> -		PCI_VEND_DEV(INTEL, IE31200_HB_8), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> -		IE31200},
> -	{
> -		PCI_VEND_DEV(INTEL, IE31200_HB_9), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> -		IE31200},
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_1), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  IE31200 },
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_2), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  IE31200 },
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_3), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  IE31200 },
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_4), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  IE31200 },
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_5), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  IE31200 },
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_6), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  IE31200 },
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_7), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  IE31200 },
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_8), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  IE31200 },
> +	{ PCI_VEND_DEV(INTEL, IE31200_HB_9), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  IE31200 },

Are these lines just changing the formatting from three lines
per entry to two?

I'm not opposed to this cleanup, but it isn't mentioned in the
commit message.  If you *really* want to make this prettier
then a helper macro:

#define PCI_DEV_ENTRY(did, chip) PCI_VEND_DEV(INTEL, did), PCI_ANY_ID, PCI_ANY_ID, 0, 0, chip

would make it look like this:

static const struct pci_device_id ie31200_pci_tbl[] = {
	{ PCI_DEV_ENTRY(IE31200_HB_1, IE31200) },
	...
	{ PCI_DEV_ENTRY(IE31200_HB_CFL_10, IE31200) },
	{ 0, } /* 0 terminated list. */
};

Then make one patch that does the cleanup by adding the macro
and using it for existing entry (marked "no functional change").

Second patch to add the new bits for Coffee Lake.

Thanks

-Tony

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] EDAC, ie31200: Add Intel Coffee Lake CPU support
  2019-06-10 18:01 ` Luck, Tony
@ 2019-06-10 18:37   ` Marco Elver
  2019-06-10 18:54     ` Borislav Petkov
  0 siblings, 1 reply; 4+ messages in thread
From: Marco Elver @ 2019-06-10 18:37 UTC (permalink / raw)
  To: Luck, Tony
  Cc: jbaron, LKML, Borislav Petkov, Mauro Carvalho Chehab, linux-edac

On Mon, 10 Jun 2019 at 20:01, Luck, Tony <tony.luck@intel.com> wrote:
>
> On Sun, Jun 09, 2019 at 05:16:13PM +0200, Marco Elver wrote:
>
> Marco,
>
> Thanks for the patch. One comment below.
>
> > -     {
> > -             PCI_VEND_DEV(INTEL, IE31200_HB_1), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > -             IE31200},
> > -     {
> > -             PCI_VEND_DEV(INTEL, IE31200_HB_2), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > -             IE31200},
> > -     {
> > -             PCI_VEND_DEV(INTEL, IE31200_HB_3), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > -             IE31200},
> > -     {
> > -             PCI_VEND_DEV(INTEL, IE31200_HB_4), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > -             IE31200},
> > -     {
> > -             PCI_VEND_DEV(INTEL, IE31200_HB_5), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > -             IE31200},
> > -     {
> > -             PCI_VEND_DEV(INTEL, IE31200_HB_6), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > -             IE31200},
> > -     {
> > -             PCI_VEND_DEV(INTEL, IE31200_HB_7), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > -             IE31200},
> > -     {
> > -             PCI_VEND_DEV(INTEL, IE31200_HB_8), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > -             IE31200},
> > -     {
> > -             PCI_VEND_DEV(INTEL, IE31200_HB_9), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > -             IE31200},
> > +     { PCI_VEND_DEV(INTEL, IE31200_HB_1), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > +       IE31200 },
> > +     { PCI_VEND_DEV(INTEL, IE31200_HB_2), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > +       IE31200 },
> > +     { PCI_VEND_DEV(INTEL, IE31200_HB_3), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > +       IE31200 },
> > +     { PCI_VEND_DEV(INTEL, IE31200_HB_4), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > +       IE31200 },
> > +     { PCI_VEND_DEV(INTEL, IE31200_HB_5), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > +       IE31200 },
> > +     { PCI_VEND_DEV(INTEL, IE31200_HB_6), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > +       IE31200 },
> > +     { PCI_VEND_DEV(INTEL, IE31200_HB_7), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > +       IE31200 },
> > +     { PCI_VEND_DEV(INTEL, IE31200_HB_8), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > +       IE31200 },
> > +     { PCI_VEND_DEV(INTEL, IE31200_HB_9), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > +       IE31200 },
>
> Are these lines just changing the formatting from three lines
> per entry to two?

Yes. Originally I had a version that added the new entries in the same
style as before, but failed check_patch.pl due to exceeding 80 chars.
I'll send v2 that reverts the formatting, but has to break line after
the 2nd PCI_ANY_ID for the new entries. I'd prefer not to introduce
another macro.

Thanks,
-- Marco

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] EDAC, ie31200: Add Intel Coffee Lake CPU support
  2019-06-10 18:37   ` Marco Elver
@ 2019-06-10 18:54     ` Borislav Petkov
  0 siblings, 0 replies; 4+ messages in thread
From: Borislav Petkov @ 2019-06-10 18:54 UTC (permalink / raw)
  To: Marco Elver; +Cc: Luck, Tony, jbaron, LKML, Mauro Carvalho Chehab, linux-edac

On Mon, Jun 10, 2019 at 08:37:01PM +0200, Marco Elver wrote:
> Yes. Originally I had a version that added the new entries in the same
> style as before, but failed check_patch.pl due to exceeding 80 chars.

Don't trust checkpatch blindly, especially about this rule. It is
perfectly fine to leave a block of code like that stick out and even
make it more tight since it is very visible which column differs if you
keep the macro arguments aligned vertically:

static const struct pci_device_id ie31200_pci_tbl[] = { 
        { PCI_VEND_DEV(INTEL, IE31200_HB_1), PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
        { PCI_VEND_DEV(INTEL, IE31200_HB_2), PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
        { PCI_VEND_DEV(INTEL, IE31200_HB_3), PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
        { PCI_VEND_DEV(INTEL, IE31200_HB_4), PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
	...

your new additions would then need to do:

	...
        { PCI_VEND_DEV(INTEL, IE31200_HB_4),     PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
	{ PCI_VEND_DEV(INTEL, IE31200_HB_CFL_1), PCI_ANY_ID, PCI_ANY_ID, 0, 0, IE31200 },
	...

to keep that vertical alignment.

> I'll send v2 that reverts the formatting, but has to break line after
> the 2nd PCI_ANY_ID for the new entries. I'd prefer not to introduce
> another macro.

Yes, but as Tony said, keep formatting changes separate from the patch
adding the Coffee Lake support.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-06-10 18:54 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-09 15:16 [PATCH] EDAC, ie31200: Add Intel Coffee Lake CPU support Marco Elver
2019-06-10 18:01 ` Luck, Tony
2019-06-10 18:37   ` Marco Elver
2019-06-10 18:54     ` Borislav Petkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).