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 <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: linux-edac <[email protected]>
---
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
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
On Mon, 10 Jun 2019 at 20:01, Luck, Tony <[email protected]> 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
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.