[PATCH 00/14] EDAC/amd64: move platform specific routines to pvt->ops

[PATCH 00/14] EDAC/amd64: move platform specific routines to pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

amd64_edac module supports CPU family/models ranging from 10h ~ 19h.

Define family specific ops routines to make the code scale well for
various platforms, also ease adding future platforms support.

The code in this patchset is partly reviewed
https://patchwork.kernel.org/project/linux-edac/cover/20220203174942.31630-1-nchatrad@amd.com/

1/14 patch in this series is the 4/12th patch in previous v7 series
[4] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-5-nchatrad@amd.com/

[2 ~ 14]/14 patches are created by splitting the 5/12th patch in
previous v7 series
[5] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

This patchset applies on top of the upstream linux kernel.
Each patch was built and tested individually. Tested the functionality
on Rome and Milan platforms.

Muralidhara M K (14):
  EDAC/amd64: Move struct fam_type variables into struct amd64_pvt
  EDAC/amd64: Add get_base_mask() into pvt->ops
  EDAC/amd64: Add prep_chip_selects() into pvt->ops
  EDAC/amd64: Add determine_memory_type() into pvt->ops
  EDAC/amd64: Add get_ecc_sym_sz() into pvt->ops
  EDAC/amd64: Add get_mc_regs() into pvt->ops
  EDAC/amd64: Add ecc_enabled() into pvt->ops
  EDAC/amd64: Add determine_edac_cap() into pvt->ops
  EDAC/amd64: Add determine_edac_ctl_cap() into pvt->ops
  EDAC/amd64: Add setup_mci_misc_sttr() into pvt->ops
  EDAC/amd64: Add populate_csrows() into pvt->ops
  EDAC/amd64: Add dump_misc_regs() into pvt->ops
  EDAC/amd64: Add get_cs_mode() into pvt->ops
  EDAC/amd64: Add get_umc_error_info() into pvt->ops

 drivers/edac/amd64_edac.c | 793 +++++++++++++++++++-------------------
 drivers/edac/amd64_edac.h |  55 ++-
 2 files changed, 419 insertions(+), 429 deletions(-)

-- 
2.25.1

[PATCH 01/14] EDAC/amd64: Move struct fam_type variables into struct amd64_pvt

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Move struct low_ops routines and members of struct amd64_family_type
to struct amd64_pvt.

This change makes code readable and simplifies adding support for
various families/models.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is the 4/12th patch in series
[v7 4/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-5-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 339 ++++++++++++--------------------------
 drivers/edac/amd64_edac.h |  42 ++---
 2 files changed, 114 insertions(+), 267 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index af2c578f8ab3..b21f43a3ec98 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -13,8 +13,6 @@ module_param(ecc_enable_override, int, 0644);
 
 static struct msr __percpu *msrs;
 
-static struct amd64_family_type *fam_type;
-
 /* Per-node stuff */
 static struct ecc_settings **ecc_stngs;
 
@@ -448,7 +446,7 @@ static void get_cs_base_and_mask(struct amd64_pvt *pvt, int csrow, u8 dct,
 	for (i = 0; i < pvt->csels[dct].m_cnt; i++)
 
 #define for_each_umc(i) \
-	for (i = 0; i < fam_type->max_mcs; i++)
+	for (i = 0; i < pvt->max_mcs; i++)
 
 /*
  * @input_addr is an InputAddr associated with the node given by mci. Return the
@@ -2787,166 +2785,6 @@ static void debug_display_dimm_sizes(struct amd64_pvt *pvt, u8 ctrl)
 	}
 }
 
-static struct amd64_family_type family_types[] = {
-	[K8_CPUS] = {
-		.ctl_name = "K8",
-		.f1_id = PCI_DEVICE_ID_AMD_K8_NB_ADDRMAP,
-		.f2_id = PCI_DEVICE_ID_AMD_K8_NB_MEMCTL,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= k8_early_channel_count,
-			.map_sysaddr_to_csrow	= k8_map_sysaddr_to_csrow,
-			.dbam_to_cs		= k8_dbam_to_chip_select,
-		}
-	},
-	[F10_CPUS] = {
-		.ctl_name = "F10h",
-		.f1_id = PCI_DEVICE_ID_AMD_10H_NB_MAP,
-		.f2_id = PCI_DEVICE_ID_AMD_10H_NB_DRAM,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f1x_early_channel_count,
-			.map_sysaddr_to_csrow	= f1x_map_sysaddr_to_csrow,
-			.dbam_to_cs		= f10_dbam_to_chip_select,
-		}
-	},
-	[F15_CPUS] = {
-		.ctl_name = "F15h",
-		.f1_id = PCI_DEVICE_ID_AMD_15H_NB_F1,
-		.f2_id = PCI_DEVICE_ID_AMD_15H_NB_F2,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f1x_early_channel_count,
-			.map_sysaddr_to_csrow	= f1x_map_sysaddr_to_csrow,
-			.dbam_to_cs		= f15_dbam_to_chip_select,
-		}
-	},
-	[F15_M30H_CPUS] = {
-		.ctl_name = "F15h_M30h",
-		.f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1,
-		.f2_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F2,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f1x_early_channel_count,
-			.map_sysaddr_to_csrow	= f1x_map_sysaddr_to_csrow,
-			.dbam_to_cs		= f16_dbam_to_chip_select,
-		}
-	},
-	[F15_M60H_CPUS] = {
-		.ctl_name = "F15h_M60h",
-		.f1_id = PCI_DEVICE_ID_AMD_15H_M60H_NB_F1,
-		.f2_id = PCI_DEVICE_ID_AMD_15H_M60H_NB_F2,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f1x_early_channel_count,
-			.map_sysaddr_to_csrow	= f1x_map_sysaddr_to_csrow,
-			.dbam_to_cs		= f15_m60h_dbam_to_chip_select,
-		}
-	},
-	[F16_CPUS] = {
-		.ctl_name = "F16h",
-		.f1_id = PCI_DEVICE_ID_AMD_16H_NB_F1,
-		.f2_id = PCI_DEVICE_ID_AMD_16H_NB_F2,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f1x_early_channel_count,
-			.map_sysaddr_to_csrow	= f1x_map_sysaddr_to_csrow,
-			.dbam_to_cs		= f16_dbam_to_chip_select,
-		}
-	},
-	[F16_M30H_CPUS] = {
-		.ctl_name = "F16h_M30h",
-		.f1_id = PCI_DEVICE_ID_AMD_16H_M30H_NB_F1,
-		.f2_id = PCI_DEVICE_ID_AMD_16H_M30H_NB_F2,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f1x_early_channel_count,
-			.map_sysaddr_to_csrow	= f1x_map_sysaddr_to_csrow,
-			.dbam_to_cs		= f16_dbam_to_chip_select,
-		}
-	},
-	[F17_CPUS] = {
-		.ctl_name = "F17h",
-		.f0_id = PCI_DEVICE_ID_AMD_17H_DF_F0,
-		.f6_id = PCI_DEVICE_ID_AMD_17H_DF_F6,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f17_early_channel_count,
-			.dbam_to_cs		= f17_addr_mask_to_cs_size,
-		}
-	},
-	[F17_M10H_CPUS] = {
-		.ctl_name = "F17h_M10h",
-		.f0_id = PCI_DEVICE_ID_AMD_17H_M10H_DF_F0,
-		.f6_id = PCI_DEVICE_ID_AMD_17H_M10H_DF_F6,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f17_early_channel_count,
-			.dbam_to_cs		= f17_addr_mask_to_cs_size,
-		}
-	},
-	[F17_M30H_CPUS] = {
-		.ctl_name = "F17h_M30h",
-		.f0_id = PCI_DEVICE_ID_AMD_17H_M30H_DF_F0,
-		.f6_id = PCI_DEVICE_ID_AMD_17H_M30H_DF_F6,
-		.max_mcs = 8,
-		.ops = {
-			.early_channel_count	= f17_early_channel_count,
-			.dbam_to_cs		= f17_addr_mask_to_cs_size,
-		}
-	},
-	[F17_M60H_CPUS] = {
-		.ctl_name = "F17h_M60h",
-		.f0_id = PCI_DEVICE_ID_AMD_17H_M60H_DF_F0,
-		.f6_id = PCI_DEVICE_ID_AMD_17H_M60H_DF_F6,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f17_early_channel_count,
-			.dbam_to_cs		= f17_addr_mask_to_cs_size,
-		}
-	},
-	[F17_M70H_CPUS] = {
-		.ctl_name = "F17h_M70h",
-		.f0_id = PCI_DEVICE_ID_AMD_17H_M70H_DF_F0,
-		.f6_id = PCI_DEVICE_ID_AMD_17H_M70H_DF_F6,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f17_early_channel_count,
-			.dbam_to_cs		= f17_addr_mask_to_cs_size,
-		}
-	},
-	[F19_CPUS] = {
-		.ctl_name = "F19h",
-		.f0_id = PCI_DEVICE_ID_AMD_19H_DF_F0,
-		.f6_id = PCI_DEVICE_ID_AMD_19H_DF_F6,
-		.max_mcs = 8,
-		.ops = {
-			.early_channel_count	= f17_early_channel_count,
-			.dbam_to_cs		= f17_addr_mask_to_cs_size,
-		}
-	},
-	[F19_M10H_CPUS] = {
-		.ctl_name = "F19h_M10h",
-		.f0_id = PCI_DEVICE_ID_AMD_19H_M10H_DF_F0,
-		.f6_id = PCI_DEVICE_ID_AMD_19H_M10H_DF_F6,
-		.max_mcs = 12,
-		.ops = {
-			.early_channel_count	= f17_early_channel_count,
-			.dbam_to_cs		= f17_addr_mask_to_cs_size,
-		}
-	},
-	[F19_M50H_CPUS] = {
-		.ctl_name = "F19h_M50h",
-		.f0_id = PCI_DEVICE_ID_AMD_19H_M50H_DF_F0,
-		.f6_id = PCI_DEVICE_ID_AMD_19H_M50H_DF_F6,
-		.max_mcs = 2,
-		.ops = {
-			.early_channel_count	= f17_early_channel_count,
-			.dbam_to_cs		= f17_addr_mask_to_cs_size,
-		}
-	},
-};
-
 /*
  * These are tables of eigenvectors (one per line) which can be used for the
  * construction of the syndrome tables. The modified syndrome search algorithm
@@ -3896,7 +3734,7 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci)
 
 	mci->edac_cap		= determine_edac_cap(pvt);
 	mci->mod_name		= EDAC_MOD_STR;
-	mci->ctl_name		= fam_type->ctl_name;
+	mci->ctl_name		= pvt->ctl_name;
 	mci->dev_name		= pci_name(pvt->F3);
 	mci->ctl_page_to_phys	= NULL;
 
@@ -3908,111 +3746,143 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci)
 /*
  * returns a pointer to the family descriptor on success, NULL otherwise.
  */
-static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt)
+static int per_family_init(struct amd64_pvt *pvt)
 {
 	pvt->ext_model  = boot_cpu_data.x86_model >> 4;
 	pvt->stepping	= boot_cpu_data.x86_stepping;
 	pvt->model	= boot_cpu_data.x86_model;
 	pvt->fam	= boot_cpu_data.x86;
+	pvt->max_mcs	= 2;
 
 	switch (pvt->fam) {
 	case 0xf:
-		fam_type	= &family_types[K8_CPUS];
-		pvt->ops	= &family_types[K8_CPUS].ops;
+		pvt->ctl_name				= (pvt->ext_model > K8_REV_F) ?
+							  "K8 revF or later" : "K8 revE or earlier";
+		pvt->f1_id				= PCI_DEVICE_ID_AMD_K8_NB_ADDRMAP;
+		pvt->f2_id				= PCI_DEVICE_ID_AMD_K8_NB_MEMCTL;
+		pvt->ops->early_channel_count		= k8_early_channel_count;
+		pvt->ops->map_sysaddr_to_csrow		= k8_map_sysaddr_to_csrow;
+		pvt->ops->dbam_to_cs			= k8_dbam_to_chip_select;
 		break;
 
 	case 0x10:
-		fam_type	= &family_types[F10_CPUS];
-		pvt->ops	= &family_types[F10_CPUS].ops;
+		pvt->ctl_name				= "F10h";
+		pvt->f1_id				= PCI_DEVICE_ID_AMD_10H_NB_MAP;
+		pvt->f2_id				= PCI_DEVICE_ID_AMD_10H_NB_DRAM;
+		pvt->ops->early_channel_count		= f1x_early_channel_count;
+		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
+		pvt->ops->dbam_to_cs			= f10_dbam_to_chip_select;
 		break;
 
 	case 0x15:
 		if (pvt->model == 0x30) {
-			fam_type = &family_types[F15_M30H_CPUS];
-			pvt->ops = &family_types[F15_M30H_CPUS].ops;
-			break;
+			pvt->ctl_name			= "F15h_M30h";
+			pvt->f1_id			= PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
+			pvt->f2_id			= PCI_DEVICE_ID_AMD_15H_M30H_NB_F2;
+			pvt->ops->dbam_to_cs		= f16_dbam_to_chip_select;
 		} else if (pvt->model == 0x60) {
-			fam_type = &family_types[F15_M60H_CPUS];
-			pvt->ops = &family_types[F15_M60H_CPUS].ops;
-			break;
-		/* Richland is only client */
+			pvt->ctl_name			= "F15h_M60h";
+			pvt->f1_id			= PCI_DEVICE_ID_AMD_15H_M60H_NB_F1;
+			pvt->f2_id			= PCI_DEVICE_ID_AMD_15H_M60H_NB_F2;
+			pvt->ops->dbam_to_cs		= f15_m60h_dbam_to_chip_select;
 		} else if (pvt->model == 0x13) {
-			return NULL;
+		/* Richland is only client */
+			return -ENODEV;
 		} else {
-			fam_type	= &family_types[F15_CPUS];
-			pvt->ops	= &family_types[F15_CPUS].ops;
+			pvt->ctl_name			= "F15h";
+			pvt->f1_id			= PCI_DEVICE_ID_AMD_15H_NB_F1;
+			pvt->f2_id			= PCI_DEVICE_ID_AMD_15H_NB_F2;
+			pvt->ops->dbam_to_cs		= f15_dbam_to_chip_select;
 		}
+		pvt->ops->early_channel_count		= f1x_early_channel_count;
+		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
 		break;
 
 	case 0x16:
 		if (pvt->model == 0x30) {
-			fam_type = &family_types[F16_M30H_CPUS];
-			pvt->ops = &family_types[F16_M30H_CPUS].ops;
-			break;
+			pvt->ctl_name			= "F16h_M30h";
+			pvt->f1_id			= PCI_DEVICE_ID_AMD_16H_M30H_NB_F1;
+			pvt->f2_id			= PCI_DEVICE_ID_AMD_16H_M30H_NB_F2;
+		} else {
+			pvt->ctl_name			= "F16h";
+			pvt->f1_id			= PCI_DEVICE_ID_AMD_16H_NB_F1;
+			pvt->f2_id			= PCI_DEVICE_ID_AMD_16H_NB_F2;
 		}
-		fam_type	= &family_types[F16_CPUS];
-		pvt->ops	= &family_types[F16_CPUS].ops;
+		pvt->ops->early_channel_count		= f1x_early_channel_count;
+		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
+		pvt->ops->dbam_to_cs			= f16_dbam_to_chip_select;
 		break;
 
 	case 0x17:
 		if (pvt->model >= 0x10 && pvt->model <= 0x2f) {
-			fam_type = &family_types[F17_M10H_CPUS];
-			pvt->ops = &family_types[F17_M10H_CPUS].ops;
-			break;
+			pvt->ctl_name			= "F17h_M10h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_17H_M10H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_17H_M10H_DF_F6;
 		} else if (pvt->model >= 0x30 && pvt->model <= 0x3f) {
-			fam_type = &family_types[F17_M30H_CPUS];
-			pvt->ops = &family_types[F17_M30H_CPUS].ops;
-			break;
+			pvt->ctl_name			= "F17h_M30h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_17H_M30H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_17H_M30H_DF_F6;
+			pvt->max_mcs			= 8;
 		} else if (pvt->model >= 0x60 && pvt->model <= 0x6f) {
-			fam_type = &family_types[F17_M60H_CPUS];
-			pvt->ops = &family_types[F17_M60H_CPUS].ops;
-			break;
+			pvt->ctl_name			= "F17h_M60h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_17H_M60H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_17H_M60H_DF_F6;
 		} else if (pvt->model >= 0x70 && pvt->model <= 0x7f) {
-			fam_type = &family_types[F17_M70H_CPUS];
-			pvt->ops = &family_types[F17_M70H_CPUS].ops;
-			break;
+			pvt->ctl_name			= "F17h_M70h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_17H_M70H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_17H_M70H_DF_F6;
+		} else {
+			pvt->ctl_name			= "F17h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_17H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_17H_DF_F6;
 		}
 		fallthrough;
 	case 0x18:
-		fam_type	= &family_types[F17_CPUS];
-		pvt->ops	= &family_types[F17_CPUS].ops;
+		pvt->ops->early_channel_count		= f17_early_channel_count;
+		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
 
-		if (pvt->fam == 0x18)
-			family_types[F17_CPUS].ctl_name = "F18h";
+		if (pvt->fam == 0x18) {
+			pvt->ctl_name			= "F18h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_17H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_17H_DF_F6;
+		}
 		break;
 
 	case 0x19:
 		if (pvt->model >= 0x10 && pvt->model <= 0x1f) {
-			fam_type = &family_types[F19_M10H_CPUS];
-			pvt->ops = &family_types[F19_M10H_CPUS].ops;
-			break;
+			pvt->ctl_name			= "F19h_M10h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_19H_M10H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_19H_M10H_DF_F6;
+			pvt->max_mcs			= 12;
 		} else if (pvt->model >= 0x20 && pvt->model <= 0x2f) {
-			fam_type = &family_types[F17_M70H_CPUS];
-			pvt->ops = &family_types[F17_M70H_CPUS].ops;
-			fam_type->ctl_name = "F19h_M20h";
-			break;
+			pvt->ctl_name			= "F19h_M20h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_17H_M70H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_17H_M70H_DF_F6;
 		} else if (pvt->model >= 0x50 && pvt->model <= 0x5f) {
-			fam_type = &family_types[F19_M50H_CPUS];
-			pvt->ops = &family_types[F19_M50H_CPUS].ops;
-			fam_type->ctl_name = "F19h_M50h";
-			break;
+			pvt->ctl_name			= "F19h_M50h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_19H_M50H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_19H_M50H_DF_F6;
 		} else if (pvt->model >= 0xa0 && pvt->model <= 0xaf) {
-			fam_type = &family_types[F19_M10H_CPUS];
-			pvt->ops = &family_types[F19_M10H_CPUS].ops;
-			fam_type->ctl_name = "F19h_MA0h";
-			break;
+			pvt->ctl_name			= "F19h_MA0h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_19H_M10H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_19H_M10H_DF_F6;
+			pvt->max_mcs			= 12;
+		} else {
+			pvt->ctl_name			= "F19h";
+			pvt->f0_id			= PCI_DEVICE_ID_AMD_19H_DF_F0;
+			pvt->f6_id			= PCI_DEVICE_ID_AMD_19H_DF_F6;
+			pvt->max_mcs			= 8;
 		}
-		fam_type	= &family_types[F19_CPUS];
-		pvt->ops	= &family_types[F19_CPUS].ops;
-		family_types[F19_CPUS].ctl_name = "F19h";
+		pvt->ops->early_channel_count		= f17_early_channel_count;
+		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
 		break;
 
 	default:
 		amd64_err("Unsupported family!\n");
-		return NULL;
+		return -ENODEV;
 	}
 
-	return fam_type;
+	return 0;
 }
 
 static const struct attribute_group *amd64_edac_attr_groups[] = {
@@ -4029,15 +3899,15 @@ static int hw_info_get(struct amd64_pvt *pvt)
 	int ret;
 
 	if (pvt->fam >= 0x17) {
-		pvt->umc = kcalloc(fam_type->max_mcs, sizeof(struct amd64_umc), GFP_KERNEL);
+		pvt->umc = kcalloc(pvt->max_mcs, sizeof(struct amd64_umc), GFP_KERNEL);
 		if (!pvt->umc)
 			return -ENOMEM;
 
-		pci_id1 = fam_type->f0_id;
-		pci_id2 = fam_type->f6_id;
+		pci_id1 = pvt->f0_id;
+		pci_id2 = pvt->f6_id;
 	} else {
-		pci_id1 = fam_type->f1_id;
-		pci_id2 = fam_type->f2_id;
+		pci_id1 = pvt->f1_id;
+		pci_id2 = pvt->f2_id;
 	}
 
 	ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2);
@@ -4083,7 +3953,7 @@ static int init_one_instance(struct amd64_pvt *pvt)
 	 * only one channel. Also, this simplifies handling later for the price
 	 * of a couple of KBs tops.
 	 */
-	layers[1].size = fam_type->max_mcs;
+	layers[1].size = pvt->max_mcs;
 	layers[1].is_virt_csrow = false;
 
 	mci = edac_mc_alloc(pvt->mc_node_id, ARRAY_SIZE(layers), layers, 0);
@@ -4113,7 +3983,7 @@ static bool instance_has_memory(struct amd64_pvt *pvt)
 	bool cs_enabled = false;
 	int cs = 0, dct = 0;
 
-	for (dct = 0; dct < fam_type->max_mcs; dct++) {
+	for (dct = 0; dct < pvt->max_mcs; dct++) {
 		for_each_chip_select(cs, dct, pvt)
 			cs_enabled |= csrow_enabled(cs, dct, pvt);
 	}
@@ -4142,10 +4012,13 @@ static int probe_one_instance(unsigned int nid)
 	pvt->mc_node_id	= nid;
 	pvt->F3 = F3;
 
+	pvt->ops = kzalloc(sizeof(*pvt->ops), GFP_KERNEL);
+	if (!pvt->ops)
+		goto err_out;
+
 	ret = -ENODEV;
-	fam_type = per_family_init(pvt);
-	if (!fam_type)
-		goto err_enable;
+	if (per_family_init(pvt))
+		goto err_out;
 
 	ret = hw_info_get(pvt);
 	if (ret < 0)
@@ -4183,11 +4056,7 @@ static int probe_one_instance(unsigned int nid)
 		goto err_enable;
 	}
 
-	amd64_info("%s %sdetected (node %d).\n", fam_type->ctl_name,
-		     (pvt->fam == 0xf ?
-				(pvt->ext_model >= K8_REV_F  ? "revF or later "
-							     : "revE or earlier ")
-				 : ""), pvt->mc_node_id);
+	amd64_info("%s detected (node %d).\n", pvt->ctl_name, pvt->mc_node_id);
 
 	dump_misc_regs(pvt);
 
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 352bda9803f6..1b2055af26b9 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -288,25 +288,6 @@
 
 #define UMC_SDP_INIT			BIT(31)
 
-enum amd_families {
-	K8_CPUS = 0,
-	F10_CPUS,
-	F15_CPUS,
-	F15_M30H_CPUS,
-	F15_M60H_CPUS,
-	F16_CPUS,
-	F16_M30H_CPUS,
-	F17_CPUS,
-	F17_M10H_CPUS,
-	F17_M30H_CPUS,
-	F17_M60H_CPUS,
-	F17_M70H_CPUS,
-	F19_CPUS,
-	F19_M10H_CPUS,
-	F19_M50H_CPUS,
-	NUM_FAMILIES,
-};
-
 /* Error injection control structure */
 struct error_injection {
 	u32	 section;
@@ -388,6 +369,11 @@ struct amd64_pvt {
 	/* x4, x8, or x16 syndromes in use */
 	u8 ecc_sym_sz;
 
+	const char *ctl_name;
+	u16 f0_id, f1_id, f2_id, f6_id;
+	/* Maximum number of memory controllers per die/node. */
+	u8 max_mcs;
+
 	/* place to store error injection parameters prior to issue */
 	struct error_injection injection;
 
@@ -473,19 +459,11 @@ struct ecc_settings {
  * functions and per device encoding/decoding logic.
  */
 struct low_ops {
-	int (*early_channel_count)	(struct amd64_pvt *pvt);
-	void (*map_sysaddr_to_csrow)	(struct mem_ctl_info *mci, u64 sys_addr,
-					 struct err_info *);
-	int (*dbam_to_cs)		(struct amd64_pvt *pvt, u8 dct,
-					 unsigned cs_mode, int cs_mask_nr);
-};
-
-struct amd64_family_type {
-	const char *ctl_name;
-	u16 f0_id, f1_id, f2_id, f6_id;
-	/* Maximum number of memory controllers per die/node. */
-	u8 max_mcs;
-	struct low_ops ops;
+	int  (*early_channel_count)(struct amd64_pvt *pvt);
+	void (*map_sysaddr_to_csrow)(struct mem_ctl_info *mci, u64 sys_addr,
+				     struct err_info *err);
+	int  (*dbam_to_cs)(struct amd64_pvt *pvt, u8 dct,
+			   unsigned int cs_mode, int cs_mask_nr);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 01/14] EDAC/amd64: Move struct fam_type variables into struct amd64_pvt

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:41PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>

...

> -static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt)
> +static int per_family_init(struct amd64_pvt *pvt)
>  {
>  	pvt->ext_model  = boot_cpu_data.x86_model >> 4;
>  	pvt->stepping	= boot_cpu_data.x86_stepping;
>  	pvt->model	= boot_cpu_data.x86_model;
>  	pvt->fam	= boot_cpu_data.x86;
> +	pvt->max_mcs	= 2;
>  
>  	switch (pvt->fam) {
>  	case 0xf:
> -		fam_type	= &family_types[K8_CPUS];
> -		pvt->ops	= &family_types[K8_CPUS].ops;
> +		pvt->ctl_name				= (pvt->ext_model > K8_REV_F) ?
> +							  "K8 revF or later" : "K8 revE or earlier";
> +		pvt->f1_id				= PCI_DEVICE_ID_AMD_K8_NB_ADDRMAP;
> +		pvt->f2_id				= PCI_DEVICE_ID_AMD_K8_NB_MEMCTL;
> +		pvt->ops->early_channel_count		= k8_early_channel_count;
> +		pvt->ops->map_sysaddr_to_csrow		= k8_map_sysaddr_to_csrow;
> +		pvt->ops->dbam_to_cs			= k8_dbam_to_chip_select;
>  		break;
>  
>  	case 0x10:
> -		fam_type	= &family_types[F10_CPUS];
> -		pvt->ops	= &family_types[F10_CPUS].ops;
> +		pvt->ctl_name				= "F10h";
> +		pvt->f1_id				= PCI_DEVICE_ID_AMD_10H_NB_MAP;
> +		pvt->f2_id				= PCI_DEVICE_ID_AMD_10H_NB_DRAM;
> +		pvt->ops->early_channel_count		= f1x_early_channel_count;
> +		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
> +		pvt->ops->dbam_to_cs			= f10_dbam_to_chip_select;
>  		break;
>  
>  	case 0x15:
>  		if (pvt->model == 0x30) {

I just noticed that families 15h and 16h aren't matching against a model
range. I think we should look into that.

...

> @@ -4142,10 +4012,13 @@ static int probe_one_instance(unsigned int nid)
>  	pvt->mc_node_id	= nid;
>  	pvt->F3 = F3;
>  
> +	pvt->ops = kzalloc(sizeof(*pvt->ops), GFP_KERNEL);
> +	if (!pvt->ops)
> +		goto err_out;
> +

This should goto err_enable so that pvt and above can be freed.

>  	ret = -ENODEV;
> -	fam_type = per_family_init(pvt);
> -	if (!fam_type)
> -		goto err_enable;
> +	if (per_family_init(pvt))
> +		goto err_out;
>

Same here, but pvt->ops should also be freed at this point too.
 
Thanks,
Yazen 

Re: [PATCH 01/14] EDAC/amd64: Move struct fam_type variables into struct amd64_pvt

[Borislav Petkov]

On Wed, Mar 23, 2022 at 05:19:26PM +0000, Yazen Ghannam wrote:
> I just noticed that families 15h and 16h aren't matching against a model
> range. I think we should look into that.

And while you're doing that, you could make those ranges a lot more
readable by using a second switch-case on the model. For example, with
F19h:

        case 0x19:
                switch (pvt->model) {
                case 0x10 ... 0x1f:
                        fam_type = &family_types[F19_M10H_CPUS];
                        pvt->ops = &family_types[F19_M10H_CPUS].ops;
                        break;
                case 0x20 ... 0x2f:
                        fam_type = &family_types[F17_M70H_CPUS];
                        pvt->ops = &family_types[F17_M70H_CPUS].ops;
                        fam_type->ctl_name = "F19h_M20h";
                        break;
                case 0x50 ... 0x5f:
                        fam_type = &family_types[F19_M50H_CPUS];
                        pvt->ops = &family_types[F19_M50H_CPUS].ops;
                        fam_type->ctl_name = "F19h_M50h";
                        break;
                case 0xa0 ... 0xaf:
                        fam_type = &family_types[F19_M10H_CPUS];
                        pvt->ops = &family_types[F19_M10H_CPUS].ops;
                        fam_type->ctl_name = "F19h_MA0h";
                        break;
                default:
                        fam_type = &family_types[F19_CPUS];
                        pvt->ops = &family_types[F19_CPUS].ops;
                        family_types[F19_CPUS].ctl_name = "F19h";
                        break;
                }
                break;

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 01/14] EDAC/amd64: Move struct fam_type variables into struct amd64_pvt

[Yazen Ghannam]

On Thu, Mar 31, 2022 at 05:45:39PM +0530, Chatradhi, Naveen Krishna wrote:

...

> > >   	case 0x15:
> > >   		if (pvt->model == 0x30) {
> > I just noticed that families 15h and 16h aren't matching against a model
> > range. I think we should look into that.
> 
> The diff seems fine to me.  Yazen, do you have any inputs on this ?
>

Right, I think this patch is moving the code around correctly. But the code
seems incorrect. So I think it should be fixed if it is indeed incorrect.

> > 
> > ...
> > 
> > > @@ -4142,10 +4012,13 @@ static int probe_one_instance(unsigned int nid)
> > >   	pvt->mc_node_id	= nid;
> > >   	pvt->F3 = F3;
> > > +	pvt->ops = kzalloc(sizeof(*pvt->ops), GFP_KERNEL);
> > > +	if (!pvt->ops)
> > > +		goto err_out;
> > > +
> > This should goto err_enable so that pvt and above can be freed.
> > >   	ret = -ENODEV;
> > > -	fam_type = per_family_init(pvt);
> > > -	if (!fam_type)
> > > -		goto err_enable;
> > > +	if (per_family_init(pvt))
> > > +		goto err_out;
> > > 
> > Same here, but pvt->ops should also be freed at this point too.
> 
> The hw_info_get() is not called till this point, calling goto err_enable
> would unnecessarily call hw_info_put()
> 
> in the exit path.  Better to introduce goto err_pvt and goto err_pvt_ops
> like below Or can i split the function
> 
> by moving the structure allocation part into a separate helper routine ?
> 
> @@ -4101,15 +4101,15 @@ static int probe_one_instance(unsigned int nid)
> 
>         pvt->ops = kzalloc(sizeof(*pvt->ops), GFP_KERNEL);
>         if (!pvt->ops)
> -               goto err_enable;
> +               goto err_pvt;
> 
>         ret = -ENODEV;
>         if (per_family_init(pvt))
> -               goto err_enable;
> +               goto err_pvt_ops;
> 
>         ret = hw_info_get(pvt);
>         if (ret < 0)
> -               goto err_enable;
> +               goto err_pvt_ops;
> 
>         ret = 0;
>         if (!instance_has_memory(pvt)) {
> @@ -4151,6 +4151,11 @@ static int probe_one_instance(unsigned int nid)
> 
>  err_enable:
>         hw_info_put(pvt);
> +
> +err_pvt_ops:
> +       kfree(pvt->ops);
> +
> +err_pvt:
>         kfree(pvt);
>

This seems okay to me.

Thanks,
Yazen 

[PATCH 02/14] EDAC/amd64: Add get_base_mask() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for get_base_mask() in pvt->ops
and assign family specific get_base_mask() definitions appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 22 ++++++++++++++++------
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index b21f43a3ec98..985c59d23a20 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1570,11 +1570,6 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
 {
 	int cs;
 
-	prep_chip_selects(pvt);
-
-	if (pvt->umc)
-		return read_umc_base_mask(pvt);
-
 	for_each_chip_select(cs, 0, pvt) {
 		int reg0   = DCSB0 + (cs * 4);
 		int reg1   = DCSB1 + (cs * 4);
@@ -3287,7 +3282,9 @@ static void read_mc_regs(struct amd64_pvt *pvt)
 	}
 
 skip:
-	read_dct_base_mask(pvt);
+	prep_chip_selects(pvt);
+
+	pvt->ops->get_base_mask(pvt);
 
 	determine_memory_type(pvt);
 	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
@@ -3763,6 +3760,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->early_channel_count		= k8_early_channel_count;
 		pvt->ops->map_sysaddr_to_csrow		= k8_map_sysaddr_to_csrow;
 		pvt->ops->dbam_to_cs			= k8_dbam_to_chip_select;
+		pvt->ops->get_base_mask			= read_dct_base_mask;
 		break;
 
 	case 0x10:
@@ -3772,6 +3770,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->early_channel_count		= f1x_early_channel_count;
 		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
 		pvt->ops->dbam_to_cs			= f10_dbam_to_chip_select;
+		pvt->ops->get_base_mask			= read_dct_base_mask;
 		break;
 
 	case 0x15:
@@ -3796,6 +3795,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		}
 		pvt->ops->early_channel_count		= f1x_early_channel_count;
 		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
+		pvt->ops->get_base_mask			= read_dct_base_mask;
 		break;
 
 	case 0x16:
@@ -3811,6 +3811,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->early_channel_count		= f1x_early_channel_count;
 		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
 		pvt->ops->dbam_to_cs			= f16_dbam_to_chip_select;
+		pvt->ops->get_base_mask			= read_dct_base_mask;
 		break;
 
 	case 0x17:
@@ -3840,6 +3841,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 	case 0x18:
 		pvt->ops->early_channel_count		= f17_early_channel_count;
 		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
+		pvt->ops->get_base_mask			= read_umc_base_mask;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3875,6 +3877,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		}
 		pvt->ops->early_channel_count		= f17_early_channel_count;
 		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
+		pvt->ops->get_base_mask			= read_umc_base_mask;
 		break;
 
 	default:
@@ -3882,6 +3885,13 @@ static int per_family_init(struct amd64_pvt *pvt)
 		return -ENODEV;
 	}
 
+	/* ops required for all the families */
+	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
+	    !pvt->ops->get_base_mask) {
+		edac_dbg(1, "Common helper routines not defined.\n");
+		return -EFAULT;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 1b2055af26b9..cf38367e3aa1 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -464,6 +464,7 @@ struct low_ops {
 				     struct err_info *err);
 	int  (*dbam_to_cs)(struct amd64_pvt *pvt, u8 dct,
 			   unsigned int cs_mode, int cs_mask_nr);
+	void (*get_base_mask)(struct amd64_pvt *pvt);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 02/14] EDAC/amd64: Add get_base_mask() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:42PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for get_base_mask() in pvt->ops
> and assign family specific get_base_mask() definitions appropriately.
>

The commit message should include why the change is needed and not just what
the change is. A few of my patches have similar feedback, so it's fresh in my
mind. :)
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 22 ++++++++++++++++------
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index b21f43a3ec98..985c59d23a20 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1570,11 +1570,6 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
>  {
>  	int cs;
>  
> -	prep_chip_selects(pvt);
> -
> -	if (pvt->umc)
> -		return read_umc_base_mask(pvt);
> -
>  	for_each_chip_select(cs, 0, pvt) {
>  		int reg0   = DCSB0 + (cs * 4);
>  		int reg1   = DCSB1 + (cs * 4);
> @@ -3287,7 +3282,9 @@ static void read_mc_regs(struct amd64_pvt *pvt)
>  	}
>  
>  skip:
> -	read_dct_base_mask(pvt);
> +	prep_chip_selects(pvt);
> +
> +	pvt->ops->get_base_mask(pvt);
>  
>  	determine_memory_type(pvt);
>  	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
> @@ -3763,6 +3760,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->early_channel_count		= k8_early_channel_count;
>  		pvt->ops->map_sysaddr_to_csrow		= k8_map_sysaddr_to_csrow;
>  		pvt->ops->dbam_to_cs			= k8_dbam_to_chip_select;
> +		pvt->ops->get_base_mask			= read_dct_base_mask;
>  		break;
>  
>  	case 0x10:
> @@ -3772,6 +3770,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->early_channel_count		= f1x_early_channel_count;
>  		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
>  		pvt->ops->dbam_to_cs			= f10_dbam_to_chip_select;
> +		pvt->ops->get_base_mask			= read_dct_base_mask;
>  		break;
>  
>  	case 0x15:
> @@ -3796,6 +3795,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		}
>  		pvt->ops->early_channel_count		= f1x_early_channel_count;
>  		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
> +		pvt->ops->get_base_mask			= read_dct_base_mask;
>  		break;
>  
>  	case 0x16:
> @@ -3811,6 +3811,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->early_channel_count		= f1x_early_channel_count;
>  		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
>  		pvt->ops->dbam_to_cs			= f16_dbam_to_chip_select;
> +		pvt->ops->get_base_mask			= read_dct_base_mask;
>  		break;
>  
>  	case 0x17:
> @@ -3840,6 +3841,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  	case 0x18:
>  		pvt->ops->early_channel_count		= f17_early_channel_count;
>  		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
> +		pvt->ops->get_base_mask			= read_umc_base_mask;
>  
>  		if (pvt->fam == 0x18) {
>  			pvt->ctl_name			= "F18h";
> @@ -3875,6 +3877,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		}
>  		pvt->ops->early_channel_count		= f17_early_channel_count;
>  		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
> +		pvt->ops->get_base_mask			= read_umc_base_mask;

The function pointer is get_base_mask() and the helper functions are
read_{dct,umc}_base_mask(). I think the naming should be more consistent.
Either read_base_mask()/read_{dct,umc}_base_mask() or
get_base_mask()/get_{dct,umc}_base_mask().

>  		break;
>  
>  	default:
> @@ -3882,6 +3885,13 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		return -ENODEV;
>  	}
>  
> +	/* ops required for all the families */
> +	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
> +	    !pvt->ops->get_base_mask) {
> +		edac_dbg(1, "Common helper routines not defined.\n");
> +		return -EFAULT;
> +	}
> +

I think it'd be clearer if we define an ops struct with default, "do nothing"
functions and statically include this in struct pvt. These can then be
overwritten in per_family_init(). I think this would avoid the need to check
that each function is set and the need to dynamically allocate the ops struct.

Thanks,
Yazen

Re: [PATCH 02/14] EDAC/amd64: Add get_base_mask() into pvt->ops

[Borislav Petkov]

On Wed, Mar 23, 2022 at 05:33:18PM +0000, Yazen Ghannam wrote:
> The commit message should include why the change is needed and not just what
> the change is. A few of my patches have similar feedback, so it's fresh in my
> mind. :)

Good. :-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH 03/14] EDAC/amd64: Add prep_chip_selects() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for prep_chip_selects() in pvt->ops and assign
family specific prep_chip_selects() definitions appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 68 ++++++++++++++++++++++++++++-----------
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 50 insertions(+), 19 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 985c59d23a20..708c4bbc0d1c 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1490,28 +1490,51 @@ static void dump_misc_regs(struct amd64_pvt *pvt)
 /*
  * See BKDG, F2x[1,0][5C:40], F2[1,0][6C:60]
  */
-static void prep_chip_selects(struct amd64_pvt *pvt)
+static void k8_prep_chip_selects(struct amd64_pvt *pvt)
 {
-	if (pvt->fam == 0xf && pvt->ext_model < K8_REV_F) {
-		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
-		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 8;
-	} else if (pvt->fam == 0x15 && pvt->model == 0x30) {
-		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
-		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
-	} else if (pvt->fam >= 0x17) {
-		int umc;
-
-		for_each_umc(umc) {
-			pvt->csels[umc].b_cnt = 4;
-			pvt->csels[umc].m_cnt = 2;
-		}
+	if (pvt->ext_model < K8_REV_F) {
+		pvt->csels[0].b_cnt = 8;
+		pvt->csels[1].b_cnt = 8;
+
+		pvt->csels[0].m_cnt = 8;
+		pvt->csels[1].m_cnt = 8;
+	} else if (pvt->ext_model >= K8_REV_F) {
+		pvt->csels[0].b_cnt = 8;
+		pvt->csels[1].b_cnt = 8;
+
+		pvt->csels[0].m_cnt = 4;
+		pvt->csels[1].m_cnt = 4;
+	}
+}
 
-	} else {
-		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
-		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
+static void f15_m30h_prep_chip_selects(struct amd64_pvt *pvt)
+{
+	pvt->csels[0].b_cnt = 4;
+	pvt->csels[1].b_cnt = 4;
+
+	pvt->csels[0].m_cnt = 2;
+	pvt->csels[1].m_cnt = 2;
+}
+
+static void f17_prep_chip_selects(struct amd64_pvt *pvt)
+{
+	int umc;
+
+	for_each_umc(umc) {
+		pvt->csels[umc].b_cnt = 4;
+		pvt->csels[umc].m_cnt = 2;
 	}
 }
 
+static void default_prep_chip_selects(struct amd64_pvt *pvt)
+{
+	pvt->csels[0].b_cnt = 8;
+	pvt->csels[1].b_cnt = 8;
+
+	pvt->csels[0].m_cnt = 4;
+	pvt->csels[1].m_cnt = 4;
+}
+
 static void read_umc_base_mask(struct amd64_pvt *pvt)
 {
 	u32 umc_base_reg, umc_base_reg_sec;
@@ -3282,7 +3305,7 @@ static void read_mc_regs(struct amd64_pvt *pvt)
 	}
 
 skip:
-	prep_chip_selects(pvt);
+	pvt->ops->prep_chip_selects(pvt);
 
 	pvt->ops->get_base_mask(pvt);
 
@@ -3761,6 +3784,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->map_sysaddr_to_csrow		= k8_map_sysaddr_to_csrow;
 		pvt->ops->dbam_to_cs			= k8_dbam_to_chip_select;
 		pvt->ops->get_base_mask			= read_dct_base_mask;
+		pvt->ops->prep_chip_selects		= k8_prep_chip_selects;
 		break;
 
 	case 0x10:
@@ -3771,6 +3795,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
 		pvt->ops->dbam_to_cs			= f10_dbam_to_chip_select;
 		pvt->ops->get_base_mask			= read_dct_base_mask;
+		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
 		break;
 
 	case 0x15:
@@ -3779,11 +3804,13 @@ static int per_family_init(struct amd64_pvt *pvt)
 			pvt->f1_id			= PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
 			pvt->f2_id			= PCI_DEVICE_ID_AMD_15H_M30H_NB_F2;
 			pvt->ops->dbam_to_cs		= f16_dbam_to_chip_select;
+			pvt->ops->prep_chip_selects	= f15_m30h_prep_chip_selects;
 		} else if (pvt->model == 0x60) {
 			pvt->ctl_name			= "F15h_M60h";
 			pvt->f1_id			= PCI_DEVICE_ID_AMD_15H_M60H_NB_F1;
 			pvt->f2_id			= PCI_DEVICE_ID_AMD_15H_M60H_NB_F2;
 			pvt->ops->dbam_to_cs		= f15_m60h_dbam_to_chip_select;
+			pvt->ops->prep_chip_selects	= default_prep_chip_selects;
 		} else if (pvt->model == 0x13) {
 		/* Richland is only client */
 			return -ENODEV;
@@ -3812,6 +3839,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
 		pvt->ops->dbam_to_cs			= f16_dbam_to_chip_select;
 		pvt->ops->get_base_mask			= read_dct_base_mask;
+		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
 		break;
 
 	case 0x17:
@@ -3842,6 +3870,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->early_channel_count		= f17_early_channel_count;
 		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
 		pvt->ops->get_base_mask			= read_umc_base_mask;
+		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3878,6 +3907,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->early_channel_count		= f17_early_channel_count;
 		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
 		pvt->ops->get_base_mask			= read_umc_base_mask;
+		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
 		break;
 
 	default:
@@ -3887,7 +3917,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 
 	/* ops required for all the families */
 	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
-	    !pvt->ops->get_base_mask) {
+	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index cf38367e3aa1..cca59a1b3021 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -465,6 +465,7 @@ struct low_ops {
 	int  (*dbam_to_cs)(struct amd64_pvt *pvt, u8 dct,
 			   unsigned int cs_mode, int cs_mask_nr);
 	void (*get_base_mask)(struct amd64_pvt *pvt);
+	void (*prep_chip_selects)(struct amd64_pvt *pvt);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 03/14] EDAC/amd64: Add prep_chip_selects() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:43PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for prep_chip_selects() in pvt->ops and assign
> family specific prep_chip_selects() definitions appropriately.
>

Please include the "why" also.
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 68 ++++++++++++++++++++++++++++-----------
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 50 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 985c59d23a20..708c4bbc0d1c 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1490,28 +1490,51 @@ static void dump_misc_regs(struct amd64_pvt *pvt)
>  /*
>   * See BKDG, F2x[1,0][5C:40], F2[1,0][6C:60]
>   */
> -static void prep_chip_selects(struct amd64_pvt *pvt)
> +static void k8_prep_chip_selects(struct amd64_pvt *pvt)
>  {
> -	if (pvt->fam == 0xf && pvt->ext_model < K8_REV_F) {
> -		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
> -		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 8;
> -	} else if (pvt->fam == 0x15 && pvt->model == 0x30) {
> -		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
> -		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
> -	} else if (pvt->fam >= 0x17) {
> -		int umc;
> -
> -		for_each_umc(umc) {
> -			pvt->csels[umc].b_cnt = 4;
> -			pvt->csels[umc].m_cnt = 2;
> -		}
> +	if (pvt->ext_model < K8_REV_F) {
> +		pvt->csels[0].b_cnt = 8;
> +		pvt->csels[1].b_cnt = 8;
> +

"b_cnt" is the same for both conditions, so these lines can be moved out of
the if-else statements.

> +		pvt->csels[0].m_cnt = 8;
> +		pvt->csels[1].m_cnt = 8;
> +	} else if (pvt->ext_model >= K8_REV_F) {

This can just be an "else".

> +		pvt->csels[0].b_cnt = 8;
> +		pvt->csels[1].b_cnt = 8;
> +
> +		pvt->csels[0].m_cnt = 4;
> +		pvt->csels[1].m_cnt = 4;
> +	}
> +}
>  
> -	} else {
> -		pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
> -		pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
> +static void f15_m30h_prep_chip_selects(struct amd64_pvt *pvt)
> +{
> +	pvt->csels[0].b_cnt = 4;
> +	pvt->csels[1].b_cnt = 4;
> +
> +	pvt->csels[0].m_cnt = 2;
> +	pvt->csels[1].m_cnt = 2;

Here, above, and below you can keep the single line style when setting
variables to the same value.

> +}
> +
> +static void f17_prep_chip_selects(struct amd64_pvt *pvt)
> +{
> +	int umc;
> +
> +	for_each_umc(umc) {
> +		pvt->csels[umc].b_cnt = 4;
> +		pvt->csels[umc].m_cnt = 2;
>  	}
>  }
>  
> +static void default_prep_chip_selects(struct amd64_pvt *pvt)
> +{
> +	pvt->csels[0].b_cnt = 8;
> +	pvt->csels[1].b_cnt = 8;
> +
> +	pvt->csels[0].m_cnt = 4;
> +	pvt->csels[1].m_cnt = 4;
> +}
> +

This may be a good example of a default (though not "do nothing") function
that can be set and overwritten when needed. That would save the NULL function
pointer check and all the lines where the pointer gets set to this default
function.

>  static void read_umc_base_mask(struct amd64_pvt *pvt)
>  {
>  	u32 umc_base_reg, umc_base_reg_sec;
> @@ -3282,7 +3305,7 @@ static void read_mc_regs(struct amd64_pvt *pvt)
>  	}
>  
>  skip:
> -	prep_chip_selects(pvt);
> +	pvt->ops->prep_chip_selects(pvt);
>  
>  	pvt->ops->get_base_mask(pvt);
>  
> @@ -3761,6 +3784,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->map_sysaddr_to_csrow		= k8_map_sysaddr_to_csrow;
>  		pvt->ops->dbam_to_cs			= k8_dbam_to_chip_select;
>  		pvt->ops->get_base_mask			= read_dct_base_mask;
> +		pvt->ops->prep_chip_selects		= k8_prep_chip_selects;
>  		break;
>  
>  	case 0x10:
> @@ -3771,6 +3795,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
>  		pvt->ops->dbam_to_cs			= f10_dbam_to_chip_select;
>  		pvt->ops->get_base_mask			= read_dct_base_mask;
> +		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
>  		break;
>  
>  	case 0x15:
> @@ -3779,11 +3804,13 @@ static int per_family_init(struct amd64_pvt *pvt)
>  			pvt->f1_id			= PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
>  			pvt->f2_id			= PCI_DEVICE_ID_AMD_15H_M30H_NB_F2;
>  			pvt->ops->dbam_to_cs		= f16_dbam_to_chip_select;
> +			pvt->ops->prep_chip_selects	= f15_m30h_prep_chip_selects;
>  		} else if (pvt->model == 0x60) {
>  			pvt->ctl_name			= "F15h_M60h";
>  			pvt->f1_id			= PCI_DEVICE_ID_AMD_15H_M60H_NB_F1;
>  			pvt->f2_id			= PCI_DEVICE_ID_AMD_15H_M60H_NB_F2;
>  			pvt->ops->dbam_to_cs		= f15_m60h_dbam_to_chip_select;
> +			pvt->ops->prep_chip_selects	= default_prep_chip_selects;
>  		} else if (pvt->model == 0x13) {
>  		/* Richland is only client */
>  			return -ENODEV;
> @@ -3812,6 +3839,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
>  		pvt->ops->dbam_to_cs			= f16_dbam_to_chip_select;
>  		pvt->ops->get_base_mask			= read_dct_base_mask;
> +		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
>  		break;
>  
>  	case 0x17:
> @@ -3842,6 +3870,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->early_channel_count		= f17_early_channel_count;
>  		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
>  		pvt->ops->get_base_mask			= read_umc_base_mask;
> +		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
>  
>  		if (pvt->fam == 0x18) {
>  			pvt->ctl_name			= "F18h";
> @@ -3878,6 +3907,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->early_channel_count		= f17_early_channel_count;
>  		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
>  		pvt->ops->get_base_mask			= read_umc_base_mask;
> +		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
>  		break;
>  

I expect that all the Zen-based CPU ops will be the same. Also, I figure that
Zen-based CPUs are likely the majority of AMD-based systems in use today, or
at least those that will use updated kernel versions. So I think that the
Zen-based CPU ops should be the default. GPU and legacy CPU ops can be set
as needed during init time.

Thanks,
Yazen 

[PATCH 04/14] EDAC/amd64: Add determine_memory_type() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for determine_memory_type() in pvt->ops and
assign family specific determine_memory_type() definitions
appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 33 ++++++++++++++++++++-------------
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 708c4bbc0d1c..07428a6c7683 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1632,20 +1632,10 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
 	}
 }
 
-static void determine_memory_type(struct amd64_pvt *pvt)
+static void f1x_determine_memory_type(struct amd64_pvt *pvt)
 {
 	u32 dram_ctrl, dcsm;
 
-	if (pvt->umc) {
-		if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(5))
-			pvt->dram_type = MEM_LRDDR4;
-		else if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(4))
-			pvt->dram_type = MEM_RDDR4;
-		else
-			pvt->dram_type = MEM_DDR4;
-		return;
-	}
-
 	switch (pvt->fam) {
 	case 0xf:
 		if (pvt->ext_model >= K8_REV_F)
@@ -1701,6 +1691,16 @@ static void determine_memory_type(struct amd64_pvt *pvt)
 	pvt->dram_type = (pvt->dclr0 & BIT(16)) ? MEM_DDR3 : MEM_RDDR3;
 }
 
+static void f17_determine_memory_type(struct amd64_pvt *pvt)
+{
+	if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(5))
+		pvt->dram_type = MEM_LRDDR4;
+	else if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(4))
+		pvt->dram_type = MEM_RDDR4;
+	else
+		pvt->dram_type = MEM_DDR4;
+}
+
 /* Get the number of DCT channels the memory controller is using. */
 static int k8_early_channel_count(struct amd64_pvt *pvt)
 {
@@ -3309,7 +3309,7 @@ static void read_mc_regs(struct amd64_pvt *pvt)
 
 	pvt->ops->get_base_mask(pvt);
 
-	determine_memory_type(pvt);
+	pvt->ops->determine_memory_type(pvt);
 	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
 
 	determine_ecc_sym_sz(pvt);
@@ -3785,6 +3785,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->dbam_to_cs			= k8_dbam_to_chip_select;
 		pvt->ops->get_base_mask			= read_dct_base_mask;
 		pvt->ops->prep_chip_selects		= k8_prep_chip_selects;
+		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		break;
 
 	case 0x10:
@@ -3796,6 +3797,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->dbam_to_cs			= f10_dbam_to_chip_select;
 		pvt->ops->get_base_mask			= read_dct_base_mask;
 		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
+		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		break;
 
 	case 0x15:
@@ -3823,6 +3825,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->early_channel_count		= f1x_early_channel_count;
 		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
 		pvt->ops->get_base_mask			= read_dct_base_mask;
+		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		break;
 
 	case 0x16:
@@ -3840,6 +3843,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->dbam_to_cs			= f16_dbam_to_chip_select;
 		pvt->ops->get_base_mask			= read_dct_base_mask;
 		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
+		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		break;
 
 	case 0x17:
@@ -3871,6 +3875,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
 		pvt->ops->get_base_mask			= read_umc_base_mask;
 		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
+		pvt->ops->determine_memory_type		= f17_determine_memory_type;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3908,6 +3913,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
 		pvt->ops->get_base_mask			= read_umc_base_mask;
 		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
+		pvt->ops->determine_memory_type		= f17_determine_memory_type;
 		break;
 
 	default:
@@ -3917,7 +3923,8 @@ static int per_family_init(struct amd64_pvt *pvt)
 
 	/* ops required for all the families */
 	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
-	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects) {
+	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects ||
+	    !pvt->ops->determine_memory_type) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index cca59a1b3021..4d8830b8afa2 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -466,6 +466,7 @@ struct low_ops {
 			   unsigned int cs_mode, int cs_mask_nr);
 	void (*get_base_mask)(struct amd64_pvt *pvt);
 	void (*prep_chip_selects)(struct amd64_pvt *pvt);
+	void (*determine_memory_type)(struct amd64_pvt *pvt);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 04/14] EDAC/amd64: Add determine_memory_type() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:44PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for determine_memory_type() in pvt->ops and
> assign family specific determine_memory_type() definitions
> appropriately.
>

Please include the "why".
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 33 ++++++++++++++++++++-------------
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 708c4bbc0d1c..07428a6c7683 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1632,20 +1632,10 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
>  	}
>  }
>  
> -static void determine_memory_type(struct amd64_pvt *pvt)
> +static void f1x_determine_memory_type(struct amd64_pvt *pvt)
>  {
>  	u32 dram_ctrl, dcsm;
>  
> -	if (pvt->umc) {
> -		if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(5))
> -			pvt->dram_type = MEM_LRDDR4;
> -		else if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(4))
> -			pvt->dram_type = MEM_RDDR4;
> -		else
> -			pvt->dram_type = MEM_DDR4;
> -		return;
> -	}
> -
>  	switch (pvt->fam) {
>  	case 0xf:
>  		if (pvt->ext_model >= K8_REV_F)
> @@ -1701,6 +1691,16 @@ static void determine_memory_type(struct amd64_pvt *pvt)
>  	pvt->dram_type = (pvt->dclr0 & BIT(16)) ? MEM_DDR3 : MEM_RDDR3;
>  }
>  
> +static void f17_determine_memory_type(struct amd64_pvt *pvt)
> +{
> +	if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(5))
> +		pvt->dram_type = MEM_LRDDR4;
> +	else if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(4))
> +		pvt->dram_type = MEM_RDDR4;
> +	else
> +		pvt->dram_type = MEM_DDR4;
> +}
> +

This will need some rework due to upstream changes.

Thanks,
Yazen

[PATCH 05/14] EDAC/amd64: Add get_ecc_sym_sz() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for determine_ecc_sym_sz() in pvt->ops and assign
family specific get_ecc_sym_sz() definitions appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 49 ++++++++++++++++++++++++---------------
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 07428a6c7683..69c33eb17e4f 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3176,26 +3176,11 @@ static void free_mc_sibling_devs(struct amd64_pvt *pvt)
 	}
 }
 
-static void determine_ecc_sym_sz(struct amd64_pvt *pvt)
+static void f1x_determine_ecc_sym_sz(struct amd64_pvt *pvt)
 {
 	pvt->ecc_sym_sz = 4;
 
-	if (pvt->umc) {
-		u8 i;
-
-		for_each_umc(i) {
-			/* Check enabled channels only: */
-			if (pvt->umc[i].sdp_ctrl & UMC_SDP_INIT) {
-				if (pvt->umc[i].ecc_ctrl & BIT(9)) {
-					pvt->ecc_sym_sz = 16;
-					return;
-				} else if (pvt->umc[i].ecc_ctrl & BIT(7)) {
-					pvt->ecc_sym_sz = 8;
-					return;
-				}
-			}
-		}
-	} else if (pvt->fam >= 0x10) {
+	if (pvt->fam >= 0x10) {
 		u32 tmp;
 
 		amd64_read_pci_cfg(pvt->F3, EXT_NB_MCA_CFG, &tmp);
@@ -3209,6 +3194,26 @@ static void determine_ecc_sym_sz(struct amd64_pvt *pvt)
 	}
 }
 
+static void f17_determine_ecc_sym_sz(struct amd64_pvt *pvt)
+{
+	u8 i;
+
+	pvt->ecc_sym_sz = 4;
+
+	for_each_umc(i) {
+		/* Check enabled channels only: */
+		if (pvt->umc[i].sdp_ctrl & UMC_SDP_INIT) {
+			if (pvt->umc[i].ecc_ctrl & BIT(9)) {
+				pvt->ecc_sym_sz = 16;
+				return;
+			} else if (pvt->umc[i].ecc_ctrl & BIT(7)) {
+				pvt->ecc_sym_sz = 8;
+				return;
+			}
+		}
+	}
+}
+
 /*
  * Retrieve the hardware registers of the memory controller.
  */
@@ -3312,7 +3317,7 @@ static void read_mc_regs(struct amd64_pvt *pvt)
 	pvt->ops->determine_memory_type(pvt);
 	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
 
-	determine_ecc_sym_sz(pvt);
+	pvt->ops->determine_ecc_sym_sz(pvt);
 }
 
 /*
@@ -3786,6 +3791,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_base_mask			= read_dct_base_mask;
 		pvt->ops->prep_chip_selects		= k8_prep_chip_selects;
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
+		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		break;
 
 	case 0x10:
@@ -3798,6 +3804,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_base_mask			= read_dct_base_mask;
 		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
+		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		break;
 
 	case 0x15:
@@ -3826,6 +3833,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
 		pvt->ops->get_base_mask			= read_dct_base_mask;
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
+		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		break;
 
 	case 0x16:
@@ -3844,6 +3852,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_base_mask			= read_dct_base_mask;
 		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
+		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		break;
 
 	case 0x17:
@@ -3876,6 +3885,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_base_mask			= read_umc_base_mask;
 		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
 		pvt->ops->determine_memory_type		= f17_determine_memory_type;
+		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3914,6 +3924,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_base_mask			= read_umc_base_mask;
 		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
 		pvt->ops->determine_memory_type		= f17_determine_memory_type;
+		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
 		break;
 
 	default:
@@ -3924,7 +3935,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 	/* ops required for all the families */
 	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
 	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects ||
-	    !pvt->ops->determine_memory_type) {
+	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 4d8830b8afa2..f6769148d8b7 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -467,6 +467,7 @@ struct low_ops {
 	void (*get_base_mask)(struct amd64_pvt *pvt);
 	void (*prep_chip_selects)(struct amd64_pvt *pvt);
 	void (*determine_memory_type)(struct amd64_pvt *pvt);
+	void (*determine_ecc_sym_sz)(struct amd64_pvt *pvt);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 05/14] EDAC/amd64: Add get_ecc_sym_sz() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:45PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for determine_ecc_sym_sz() in pvt->ops and assign
> family specific get_ecc_sym_sz() definitions appropriately.
>

Please include the "why".
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 49 ++++++++++++++++++++++++---------------
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 07428a6c7683..69c33eb17e4f 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3176,26 +3176,11 @@ static void free_mc_sibling_devs(struct amd64_pvt *pvt)
>  	}
>  }
>  
> -static void determine_ecc_sym_sz(struct amd64_pvt *pvt)
> +static void f1x_determine_ecc_sym_sz(struct amd64_pvt *pvt)
>  {
>  	pvt->ecc_sym_sz = 4;
>  
> -	if (pvt->umc) {
> -		u8 i;
> -
> -		for_each_umc(i) {
> -			/* Check enabled channels only: */
> -			if (pvt->umc[i].sdp_ctrl & UMC_SDP_INIT) {
> -				if (pvt->umc[i].ecc_ctrl & BIT(9)) {
> -					pvt->ecc_sym_sz = 16;
> -					return;
> -				} else if (pvt->umc[i].ecc_ctrl & BIT(7)) {
> -					pvt->ecc_sym_sz = 8;
> -					return;
> -				}
> -			}
> -		}
> -	} else if (pvt->fam >= 0x10) {
> +	if (pvt->fam >= 0x10) {
>  		u32 tmp;
>  
>  		amd64_read_pci_cfg(pvt->F3, EXT_NB_MCA_CFG, &tmp);
> @@ -3209,6 +3194,26 @@ static void determine_ecc_sym_sz(struct amd64_pvt *pvt)
>  	}
>  }
>  
> +static void f17_determine_ecc_sym_sz(struct amd64_pvt *pvt)
> +{
> +	u8 i;
> +
> +	pvt->ecc_sym_sz = 4;
> +
> +	for_each_umc(i) {
> +		/* Check enabled channels only: */
> +		if (pvt->umc[i].sdp_ctrl & UMC_SDP_INIT) {
> +			if (pvt->umc[i].ecc_ctrl & BIT(9)) {
> +				pvt->ecc_sym_sz = 16;
> +				return;
> +			} else if (pvt->umc[i].ecc_ctrl & BIT(7)) {
> +				pvt->ecc_sym_sz = 8;
> +				return;
> +			}
> +		}
> +	}
> +}

We should reconsider if this is needed. The ECC symbol size was needed on
legacy systems to decode the syndrome. The MCA_SYND register includes the
syndrome on Zen-based systems, so the symbol size is only used for a single
pr_info().

Thanks,
Yazen

[PATCH 06/14] EDAC/amd64: Add get_mc_regs() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for get_mc_regs() in pvt->ops and assign
family specific get_mc_regs() definitions appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 77 +++++++++++++++++++++------------------
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 69c33eb17e4f..713ffe763e64 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3214,6 +3214,27 @@ static void f17_determine_ecc_sym_sz(struct amd64_pvt *pvt)
 	}
 }
 
+static void read_top_mem_registers(struct amd64_pvt *pvt)
+{
+	u64 msr_val;
+
+	/*
+	 * Retrieve TOP_MEM and TOP_MEM2; no masking off of reserved bits since
+	 * those are Read-As-Zero.
+	 */
+	rdmsrl(MSR_K8_TOP_MEM1, pvt->top_mem);
+	edac_dbg(0, "  TOP_MEM:  0x%016llx\n", pvt->top_mem);
+
+	/* Check first whether TOP_MEM2 is enabled: */
+	rdmsrl(MSR_AMD64_SYSCFG, msr_val);
+	if (msr_val & BIT(21)) {
+		rdmsrl(MSR_K8_TOP_MEM2, pvt->top_mem2);
+		edac_dbg(0, "  TOP_MEM2: 0x%016llx\n", pvt->top_mem2);
+	} else {
+		edac_dbg(0, "  TOP_MEM2 disabled\n");
+	}
+}
+
 /*
  * Retrieve the hardware registers of the memory controller.
  */
@@ -3235,6 +3256,8 @@ static void __read_mc_regs_df(struct amd64_pvt *pvt)
 		amd_smn_read(nid, umc_base + UMCCH_ECC_CTRL, &umc->ecc_ctrl);
 		amd_smn_read(nid, umc_base + UMCCH_UMC_CAP_HI, &umc->umc_cap_hi);
 	}
+
+	amd64_read_pci_cfg(pvt->F0, DF_DHAR, &pvt->dhar);
 }
 
 /*
@@ -3244,30 +3267,8 @@ static void __read_mc_regs_df(struct amd64_pvt *pvt)
 static void read_mc_regs(struct amd64_pvt *pvt)
 {
 	unsigned int range;
-	u64 msr_val;
 
-	/*
-	 * Retrieve TOP_MEM and TOP_MEM2; no masking off of reserved bits since
-	 * those are Read-As-Zero.
-	 */
-	rdmsrl(MSR_K8_TOP_MEM1, pvt->top_mem);
-	edac_dbg(0, "  TOP_MEM:  0x%016llx\n", pvt->top_mem);
-
-	/* Check first whether TOP_MEM2 is enabled: */
-	rdmsrl(MSR_AMD64_SYSCFG, msr_val);
-	if (msr_val & BIT(21)) {
-		rdmsrl(MSR_K8_TOP_MEM2, pvt->top_mem2);
-		edac_dbg(0, "  TOP_MEM2: 0x%016llx\n", pvt->top_mem2);
-	} else {
-		edac_dbg(0, "  TOP_MEM2 disabled\n");
-	}
-
-	if (pvt->umc) {
-		__read_mc_regs_df(pvt);
-		amd64_read_pci_cfg(pvt->F0, DF_DHAR, &pvt->dhar);
-
-		goto skip;
-	}
+	read_top_mem_registers(pvt);
 
 	amd64_read_pci_cfg(pvt->F3, NBCAP, &pvt->nbcap);
 
@@ -3308,16 +3309,6 @@ static void read_mc_regs(struct amd64_pvt *pvt)
 		amd64_read_dct_pci_cfg(pvt, 1, DCLR0, &pvt->dclr1);
 		amd64_read_dct_pci_cfg(pvt, 1, DCHR0, &pvt->dchr1);
 	}
-
-skip:
-	pvt->ops->prep_chip_selects(pvt);
-
-	pvt->ops->get_base_mask(pvt);
-
-	pvt->ops->determine_memory_type(pvt);
-	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
-
-	pvt->ops->determine_ecc_sym_sz(pvt);
 }
 
 /*
@@ -3792,6 +3783,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->prep_chip_selects		= k8_prep_chip_selects;
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
+		pvt->ops->get_mc_regs			= read_mc_regs;
 		break;
 
 	case 0x10:
@@ -3805,6 +3797,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
+		pvt->ops->get_mc_regs			= read_mc_regs;
 		break;
 
 	case 0x15:
@@ -3834,6 +3827,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_base_mask			= read_dct_base_mask;
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
+		pvt->ops->get_mc_regs			= read_mc_regs;
 		break;
 
 	case 0x16:
@@ -3853,6 +3847,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
+		pvt->ops->get_mc_regs			= read_mc_regs;
 		break;
 
 	case 0x17:
@@ -3886,6 +3881,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
 		pvt->ops->determine_memory_type		= f17_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
+		pvt->ops->get_mc_regs			= __read_mc_regs_df;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3925,6 +3921,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
 		pvt->ops->determine_memory_type		= f17_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
+		pvt->ops->get_mc_regs			= __read_mc_regs_df;
 		break;
 
 	default:
@@ -3935,7 +3932,8 @@ static int per_family_init(struct amd64_pvt *pvt)
 	/* ops required for all the families */
 	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
 	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects ||
-	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz) {
+	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
+	    !pvt->ops->get_mc_regs) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
@@ -3972,7 +3970,16 @@ static int hw_info_get(struct amd64_pvt *pvt)
 	if (ret)
 		return ret;
 
-	read_mc_regs(pvt);
+	pvt->ops->get_mc_regs(pvt);
+
+	pvt->ops->prep_chip_selects(pvt);
+
+	pvt->ops->get_base_mask(pvt);
+
+	pvt->ops->determine_memory_type(pvt);
+	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
+
+	pvt->ops->determine_ecc_sym_sz(pvt);
 
 	return 0;
 }
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index f6769148d8b7..1b6df33bb0a8 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -468,6 +468,7 @@ struct low_ops {
 	void (*prep_chip_selects)(struct amd64_pvt *pvt);
 	void (*determine_memory_type)(struct amd64_pvt *pvt);
 	void (*determine_ecc_sym_sz)(struct amd64_pvt *pvt);
+	void (*get_mc_regs)(struct amd64_pvt *pvt);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 06/14] EDAC/amd64: Add get_mc_regs() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:46PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for get_mc_regs() in pvt->ops and assign
> family specific get_mc_regs() definitions appropriately.
> 

Please include the "why".

> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 77 +++++++++++++++++++++------------------
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 43 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 69c33eb17e4f..713ffe763e64 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3214,6 +3214,27 @@ static void f17_determine_ecc_sym_sz(struct amd64_pvt *pvt)
>  	}
>  }
>  
> +static void read_top_mem_registers(struct amd64_pvt *pvt)
> +{
> +	u64 msr_val;
> +
> +	/*
> +	 * Retrieve TOP_MEM and TOP_MEM2; no masking off of reserved bits since
> +	 * those are Read-As-Zero.
> +	 */
> +	rdmsrl(MSR_K8_TOP_MEM1, pvt->top_mem);
> +	edac_dbg(0, "  TOP_MEM:  0x%016llx\n", pvt->top_mem);
> +
> +	/* Check first whether TOP_MEM2 is enabled: */
> +	rdmsrl(MSR_AMD64_SYSCFG, msr_val);
> +	if (msr_val & BIT(21)) {
> +		rdmsrl(MSR_K8_TOP_MEM2, pvt->top_mem2);
> +		edac_dbg(0, "  TOP_MEM2: 0x%016llx\n", pvt->top_mem2);
> +	} else {
> +		edac_dbg(0, "  TOP_MEM2 disabled\n");
> +	}

These two values are not used by any code within this module. They are only
used in debug print statements and debug sysfs entries. I think this code
should just be removed. An expert user who wants to know TOM and TOM2 can use
another method, like msr-tools, rather than recompile a kernel with
CONFIG_EDAC_DEBUG, etc.

> +}
> +
>  /*
>   * Retrieve the hardware registers of the memory controller.
>   */
> @@ -3235,6 +3256,8 @@ static void __read_mc_regs_df(struct amd64_pvt *pvt)
>  		amd_smn_read(nid, umc_base + UMCCH_ECC_CTRL, &umc->ecc_ctrl);
>  		amd_smn_read(nid, umc_base + UMCCH_UMC_CAP_HI, &umc->umc_cap_hi);
>  	}
> +
> +	amd64_read_pci_cfg(pvt->F0, DF_DHAR, &pvt->dhar);

"dhar" is not used by any code for Zen-based systems. I think this line can be
dropped. Reading "dhar" should still be preserved for legacy systems.

This is also the only use of PCI F0. So all the F0 IDs can be removed too. I
have a patch for this as part of some general code clean up. Let's include
that with this set also. I think removing TOM/TOM2 code can be included too.

>  }
>  
>  /*
> @@ -3244,30 +3267,8 @@ static void __read_mc_regs_df(struct amd64_pvt *pvt)
>  static void read_mc_regs(struct amd64_pvt *pvt)
>  {
>  	unsigned int range;
> -	u64 msr_val;
>  
> -	/*
> -	 * Retrieve TOP_MEM and TOP_MEM2; no masking off of reserved bits since
> -	 * those are Read-As-Zero.
> -	 */
> -	rdmsrl(MSR_K8_TOP_MEM1, pvt->top_mem);
> -	edac_dbg(0, "  TOP_MEM:  0x%016llx\n", pvt->top_mem);
> -
> -	/* Check first whether TOP_MEM2 is enabled: */
> -	rdmsrl(MSR_AMD64_SYSCFG, msr_val);
> -	if (msr_val & BIT(21)) {
> -		rdmsrl(MSR_K8_TOP_MEM2, pvt->top_mem2);
> -		edac_dbg(0, "  TOP_MEM2: 0x%016llx\n", pvt->top_mem2);
> -	} else {
> -		edac_dbg(0, "  TOP_MEM2 disabled\n");
> -	}
> -
> -	if (pvt->umc) {
> -		__read_mc_regs_df(pvt);
> -		amd64_read_pci_cfg(pvt->F0, DF_DHAR, &pvt->dhar);
> -
> -		goto skip;
> -	}
> +	read_top_mem_registers(pvt);
>  
>  	amd64_read_pci_cfg(pvt->F3, NBCAP, &pvt->nbcap);
>  
> @@ -3308,16 +3309,6 @@ static void read_mc_regs(struct amd64_pvt *pvt)
>  		amd64_read_dct_pci_cfg(pvt, 1, DCLR0, &pvt->dclr1);
>  		amd64_read_dct_pci_cfg(pvt, 1, DCHR0, &pvt->dchr1);
>  	}
> -
> -skip:
> -	pvt->ops->prep_chip_selects(pvt);
> -
> -	pvt->ops->get_base_mask(pvt);
> -
> -	pvt->ops->determine_memory_type(pvt);
> -	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
> -
> -	pvt->ops->determine_ecc_sym_sz(pvt);
>  }
>  
>  /*
> @@ -3792,6 +3783,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->prep_chip_selects		= k8_prep_chip_selects;
>  		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>  		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
> +		pvt->ops->get_mc_regs			= read_mc_regs;

The function names should be more consistent: either both get or read.

The read_mc_regs() function is used for systems with DCTs (i.e. legacy). This
can be included in the name.

>  		break;
>  
>  	case 0x10:
> @@ -3805,6 +3797,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
>  		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>  		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
> +		pvt->ops->get_mc_regs			= read_mc_regs;
>  		break;
>  
>  	case 0x15:
> @@ -3834,6 +3827,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->get_base_mask			= read_dct_base_mask;
>  		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>  		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
> +		pvt->ops->get_mc_regs			= read_mc_regs;
>  		break;
>  
>  	case 0x16:
> @@ -3853,6 +3847,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
>  		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>  		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
> +		pvt->ops->get_mc_regs			= read_mc_regs;
>  		break;
>  
>  	case 0x17:
> @@ -3886,6 +3881,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
>  		pvt->ops->determine_memory_type		= f17_determine_memory_type;
>  		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
> +		pvt->ops->get_mc_regs			= __read_mc_regs_df;

The underscore prefix can be removed, since this is no longer a helper
function. Also, the "df" suffix can be removed when changing the name.

Maybe something like this:

pvt->ops->read_mc_regs()    <--- This reads memory controller registers.

read_dct_regs()    <--- Used for DRAM Controllers (DCTs).

read_umc_regs()    <--- Used for Unified Memory Controllers (UMCs).

>  
>  		if (pvt->fam == 0x18) {
>  			pvt->ctl_name			= "F18h";
> @@ -3925,6 +3921,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
>  		pvt->ops->determine_memory_type		= f17_determine_memory_type;
>  		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
> +		pvt->ops->get_mc_regs			= __read_mc_regs_df;
>  		break;
>  
>  	default:
> @@ -3935,7 +3932,8 @@ static int per_family_init(struct amd64_pvt *pvt)
>  	/* ops required for all the families */
>  	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
>  	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects ||
> -	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz) {
> +	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
> +	    !pvt->ops->get_mc_regs) {
>  		edac_dbg(1, "Common helper routines not defined.\n");
>  		return -EFAULT;
>  	}
> @@ -3972,7 +3970,16 @@ static int hw_info_get(struct amd64_pvt *pvt)
>  	if (ret)
>  		return ret;
>  
> -	read_mc_regs(pvt);
> +	pvt->ops->get_mc_regs(pvt);
> +
> +	pvt->ops->prep_chip_selects(pvt);
> +
> +	pvt->ops->get_base_mask(pvt);
> +
> +	pvt->ops->determine_memory_type(pvt);
> +	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);

This line should be included in determine_memory_type(). It should be called
for each PVT on legacy systems and for each UMC on current systems.

Thanks,
Yazen

Re: [PATCH 06/14] EDAC/amd64: Add get_mc_regs() into pvt->ops

[Chatradhi, Naveen Krishna]

Hi Yazen,

On 3/28/2022 9:38 PM, Yazen Ghannam wrote:
> On Mon, Feb 28, 2022 at 09:43:46PM +0530, Naveen Krishna Chatradhi wrote:
>> From: Muralidhara M K <muralimk@amd.com>
>>
>> Add function pointer for get_mc_regs() in pvt->ops and assign
>> family specific get_mc_regs() definitions appropriately.
>>
> Please include the "why".
Sure
>
>> Signed-off-by: Muralidhara M K <muralimk@amd.com>
>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>> ---
>> This patch is created by splitting the 5/12th patch in series
>> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
>>
>>   drivers/edac/amd64_edac.c | 77 +++++++++++++++++++++------------------
>>   drivers/edac/amd64_edac.h |  1 +
>>   2 files changed, 43 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
>> index 69c33eb17e4f..713ffe763e64 100644
>> --- a/drivers/edac/amd64_edac.c
>> +++ b/drivers/edac/amd64_edac.c
>> @@ -3214,6 +3214,27 @@ static void f17_determine_ecc_sym_sz(struct amd64_pvt *pvt)
>>   	}
>>   }
>>   
>> +static void read_top_mem_registers(struct amd64_pvt *pvt)
>> +{
>> +	u64 msr_val;
>> +
>> +	/*
>> +	 * Retrieve TOP_MEM and TOP_MEM2; no masking off of reserved bits since
>> +	 * those are Read-As-Zero.
>> +	 */
>> +	rdmsrl(MSR_K8_TOP_MEM1, pvt->top_mem);
>> +	edac_dbg(0, "  TOP_MEM:  0x%016llx\n", pvt->top_mem);
>> +
>> +	/* Check first whether TOP_MEM2 is enabled: */
>> +	rdmsrl(MSR_AMD64_SYSCFG, msr_val);
>> +	if (msr_val & BIT(21)) {
>> +		rdmsrl(MSR_K8_TOP_MEM2, pvt->top_mem2);
>> +		edac_dbg(0, "  TOP_MEM2: 0x%016llx\n", pvt->top_mem2);
>> +	} else {
>> +		edac_dbg(0, "  TOP_MEM2 disabled\n");
>> +	}
> These two values are not used by any code within this module. They are only
> used in debug print statements and debug sysfs entries. I think this code
> should just be removed. An expert user who wants to know TOM and TOM2 can use
> another method, like msr-tools, rather than recompile a kernel with
> CONFIG_EDAC_DEBUG, etc.
Make sense, do you think some users have developed scripts to parse the 
EDAC debug logs ?
>
>> +}
>> +
>>   /*
>>    * Retrieve the hardware registers of the memory controller.
>>    */
>> @@ -3235,6 +3256,8 @@ static void __read_mc_regs_df(struct amd64_pvt *pvt)
>>   		amd_smn_read(nid, umc_base + UMCCH_ECC_CTRL, &umc->ecc_ctrl);
>>   		amd_smn_read(nid, umc_base + UMCCH_UMC_CAP_HI, &umc->umc_cap_hi);
>>   	}
>> +
>> +	amd64_read_pci_cfg(pvt->F0, DF_DHAR, &pvt->dhar);
> "dhar" is not used by any code for Zen-based systems. I think this line can be
> dropped. Reading "dhar" should still be preserved for legacy systems.
>
> This is also the only use of PCI F0. So all the F0 IDs can be removed too. I
> have a patch for this as part of some general code clean up. Let's include
> that with this set also. I think removing TOM/TOM2 code can be included too.
Sure, we can work on this.
>
>>   }
>>   
>>   /*
>> @@ -3244,30 +3267,8 @@ static void __read_mc_regs_df(struct amd64_pvt *pvt)
>>   static void read_mc_regs(struct amd64_pvt *pvt)
>>   {
>>   	unsigned int range;
>> -	u64 msr_val;
>>   
>> -	/*
>> -	 * Retrieve TOP_MEM and TOP_MEM2; no masking off of reserved bits since
>> -	 * those are Read-As-Zero.
>> -	 */
>> -	rdmsrl(MSR_K8_TOP_MEM1, pvt->top_mem);
>> -	edac_dbg(0, "  TOP_MEM:  0x%016llx\n", pvt->top_mem);
>> -
>> -	/* Check first whether TOP_MEM2 is enabled: */
>> -	rdmsrl(MSR_AMD64_SYSCFG, msr_val);
>> -	if (msr_val & BIT(21)) {
>> -		rdmsrl(MSR_K8_TOP_MEM2, pvt->top_mem2);
>> -		edac_dbg(0, "  TOP_MEM2: 0x%016llx\n", pvt->top_mem2);
>> -	} else {
>> -		edac_dbg(0, "  TOP_MEM2 disabled\n");
>> -	}
>> -
>> -	if (pvt->umc) {
>> -		__read_mc_regs_df(pvt);
>> -		amd64_read_pci_cfg(pvt->F0, DF_DHAR, &pvt->dhar);
>> -
>> -		goto skip;
>> -	}
>> +	read_top_mem_registers(pvt);
>>   
>>   	amd64_read_pci_cfg(pvt->F3, NBCAP, &pvt->nbcap);
>>   
>> @@ -3308,16 +3309,6 @@ static void read_mc_regs(struct amd64_pvt *pvt)
>>   		amd64_read_dct_pci_cfg(pvt, 1, DCLR0, &pvt->dclr1);
>>   		amd64_read_dct_pci_cfg(pvt, 1, DCHR0, &pvt->dchr1);
>>   	}
>> -
>> -skip:
>> -	pvt->ops->prep_chip_selects(pvt);
>> -
>> -	pvt->ops->get_base_mask(pvt);
>> -
>> -	pvt->ops->determine_memory_type(pvt);
>> -	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
>> -
>> -	pvt->ops->determine_ecc_sym_sz(pvt);
>>   }
>>   
>>   /*
>> @@ -3792,6 +3783,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->prep_chip_selects		= k8_prep_chip_selects;
>>   		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= read_mc_regs;
> The function names should be more consistent: either both get or read.
>
> The read_mc_regs() function is used for systems with DCTs (i.e. legacy). This
> can be included in the name.
sure
>
>>   		break;
>>   
>>   	case 0x10:
>> @@ -3805,6 +3797,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
>>   		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= read_mc_regs;
>>   		break;
>>   
>>   	case 0x15:
>> @@ -3834,6 +3827,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->get_base_mask			= read_dct_base_mask;
>>   		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= read_mc_regs;
>>   		break;
>>   
>>   	case 0x16:
>> @@ -3853,6 +3847,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
>>   		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= read_mc_regs;
>>   		break;
>>   
>>   	case 0x17:
>> @@ -3886,6 +3881,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
>>   		pvt->ops->determine_memory_type		= f17_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= __read_mc_regs_df;
> The underscore prefix can be removed, since this is no longer a helper
> function. Also, the "df" suffix can be removed when changing the name.
>
> Maybe something like this:
>
> pvt->ops->read_mc_regs()    <--- This reads memory controller registers.
>
> read_dct_regs()    <--- Used for DRAM Controllers (DCTs).
>
> read_umc_regs()    <--- Used for Unified Memory Controllers (UMCs).
sure
>
>>   
>>   		if (pvt->fam == 0x18) {
>>   			pvt->ctl_name			= "F18h";
>> @@ -3925,6 +3921,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
>>   		pvt->ops->determine_memory_type		= f17_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= __read_mc_regs_df;
>>   		break;
>>   
>>   	default:
>> @@ -3935,7 +3932,8 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   	/* ops required for all the families */
>>   	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
>>   	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects ||
>> -	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz) {
>> +	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
>> +	    !pvt->ops->get_mc_regs) {
>>   		edac_dbg(1, "Common helper routines not defined.\n");
>>   		return -EFAULT;
>>   	}
>> @@ -3972,7 +3970,16 @@ static int hw_info_get(struct amd64_pvt *pvt)
>>   	if (ret)
>>   		return ret;
>>   
>> -	read_mc_regs(pvt);
>> +	pvt->ops->get_mc_regs(pvt);
>> +
>> +	pvt->ops->prep_chip_selects(pvt);
>> +
>> +	pvt->ops->get_base_mask(pvt);
>> +
>> +	pvt->ops->determine_memory_type(pvt);
>> +	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
> This line should be included in determine_memory_type(). It should be called
> for each PVT on legacy systems and for each UMC on current systems.

sure

Regards,

Naveenk

>
> Thanks,
> Yazen

Re: [PATCH 06/14] EDAC/amd64: Add get_mc_regs() into pvt->ops

[Yazen Ghannam]

On Thu, Mar 31, 2022 at 05:49:49PM +0530, Chatradhi, Naveen Krishna wrote:

...

> > > +static void read_top_mem_registers(struct amd64_pvt *pvt)
> > > +{
> > > +	u64 msr_val;
> > > +
> > > +	/*
> > > +	 * Retrieve TOP_MEM and TOP_MEM2; no masking off of reserved bits since
> > > +	 * those are Read-As-Zero.
> > > +	 */
> > > +	rdmsrl(MSR_K8_TOP_MEM1, pvt->top_mem);
> > > +	edac_dbg(0, "  TOP_MEM:  0x%016llx\n", pvt->top_mem);
> > > +
> > > +	/* Check first whether TOP_MEM2 is enabled: */
> > > +	rdmsrl(MSR_AMD64_SYSCFG, msr_val);
> > > +	if (msr_val & BIT(21)) {
> > > +		rdmsrl(MSR_K8_TOP_MEM2, pvt->top_mem2);
> > > +		edac_dbg(0, "  TOP_MEM2: 0x%016llx\n", pvt->top_mem2);
> > > +	} else {
> > > +		edac_dbg(0, "  TOP_MEM2 disabled\n");
> > > +	}
> > These two values are not used by any code within this module. They are only
> > used in debug print statements and debug sysfs entries. I think this code
> > should just be removed. An expert user who wants to know TOM and TOM2 can use
> > another method, like msr-tools, rather than recompile a kernel with
> > CONFIG_EDAC_DEBUG, etc.
> Make sense, do you think some users have developed scripts to parse the EDAC
> debug logs ?

I'm not aware of any users of this. But this isn't a robust interface like an
ABI.

Thanks,
Yazen

Re: [PATCH 06/14] EDAC/amd64: Add get_mc_regs() into pvt->ops

[Borislav Petkov]

On Mon, Apr 04, 2022 at 06:19:36PM +0000, Yazen Ghannam wrote:
> I'm not aware of any users of this. But this isn't a robust interface like an
> ABI.

Yes, kernel printk message formatting is not an ABI.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH 07/14] EDAC/amd64: Add ecc_enabled() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for ecc_enabled() in pvt->ops and assign
family specific ecc_enabled() definitions appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 77 ++++++++++++++++++++++++---------------
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 48 insertions(+), 30 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 713ffe763e64..15d775a9ce7e 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3649,49 +3649,60 @@ static void restore_ecc_error_reporting(struct ecc_settings *s, u16 nid,
 		amd64_warn("Error restoring NB MCGCTL settings!\n");
 }
 
-static bool ecc_enabled(struct amd64_pvt *pvt)
+static bool f1x_ecc_enabled(struct amd64_pvt *pvt)
 {
 	u16 nid = pvt->mc_node_id;
 	bool nb_mce_en = false;
-	u8 ecc_en = 0, i;
+	u8 ecc_en = 0;
 	u32 value;
 
-	if (boot_cpu_data.x86 >= 0x17) {
-		u8 umc_en_mask = 0, ecc_en_mask = 0;
-		struct amd64_umc *umc;
+	amd64_read_pci_cfg(pvt->F3, NBCFG, &value);
 
-		for_each_umc(i) {
-			umc = &pvt->umc[i];
+	ecc_en = !!(value & NBCFG_ECC_ENABLE);
 
-			/* Only check enabled UMCs. */
-			if (!(umc->sdp_ctrl & UMC_SDP_INIT))
-				continue;
+	nb_mce_en = nb_mce_bank_enabled_on_node(nid);
+	if (!nb_mce_en)
+		edac_dbg(0, "NB MCE bank disabled, set MSR 0x%08x[4] on node %d to enable.\n",
+			 MSR_IA32_MCG_CTL, nid);
 
-			umc_en_mask |= BIT(i);
+	edac_dbg(3, "Node %d: DRAM ECC %s.\n", nid, (ecc_en ? "enabled" : "disabled"));
 
-			if (umc->umc_cap_hi & UMC_ECC_ENABLED)
-				ecc_en_mask |= BIT(i);
-		}
+	if (!ecc_en || !nb_mce_en)
+		return false;
+	else
+		return true;
+}
 
-		/* Check whether at least one UMC is enabled: */
-		if (umc_en_mask)
-			ecc_en = umc_en_mask == ecc_en_mask;
-		else
-			edac_dbg(0, "Node %d: No enabled UMCs.\n", nid);
+static bool f17_ecc_enabled(struct amd64_pvt *pvt)
+{
+	u8 umc_en_mask = 0, ecc_en_mask = 0;
+	u8 ecc_en = 0, i;
+	u16 nid = pvt->mc_node_id;
+	bool nb_mce_en = false;
+	struct amd64_umc *umc;
 
-		/* Assume UMC MCA banks are enabled. */
-		nb_mce_en = true;
-	} else {
-		amd64_read_pci_cfg(pvt->F3, NBCFG, &value);
+	for_each_umc(i) {
+		umc = &pvt->umc[i];
+
+		/* Only check enabled UMCs. */
+		if (!(umc->sdp_ctrl & UMC_SDP_INIT))
+			continue;
 
-		ecc_en = !!(value & NBCFG_ECC_ENABLE);
+		umc_en_mask |= BIT(i);
 
-		nb_mce_en = nb_mce_bank_enabled_on_node(nid);
-		if (!nb_mce_en)
-			edac_dbg(0, "NB MCE bank disabled, set MSR 0x%08x[4] on node %d to enable.\n",
-				     MSR_IA32_MCG_CTL, nid);
+		if (umc->umc_cap_hi & UMC_ECC_ENABLED)
+			ecc_en_mask |= BIT(i);
 	}
 
+	/* Check whether at least one UMC is enabled: */
+	if (umc_en_mask)
+		ecc_en = umc_en_mask == ecc_en_mask;
+	else
+		edac_dbg(0, "Node %d: No enabled UMCs.\n", nid);
+
+	/* Assume UMC MCA banks are enabled. */
+	nb_mce_en = true;
+
 	edac_dbg(3, "Node %d: DRAM ECC %s.\n", nid, (ecc_en ? "enabled" : "disabled"));
 
 	if (!ecc_en || !nb_mce_en)
@@ -3784,6 +3795,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= read_mc_regs;
+		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		break;
 
 	case 0x10:
@@ -3798,6 +3810,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= read_mc_regs;
+		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		break;
 
 	case 0x15:
@@ -3828,6 +3841,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= read_mc_regs;
+		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		break;
 
 	case 0x16:
@@ -3848,6 +3862,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= read_mc_regs;
+		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		break;
 
 	case 0x17:
@@ -3882,6 +3897,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_memory_type		= f17_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= __read_mc_regs_df;
+		pvt->ops->ecc_enabled			= f17_ecc_enabled;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3922,6 +3938,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_memory_type		= f17_determine_memory_type;
 		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= __read_mc_regs_df;
+		pvt->ops->ecc_enabled			= f17_ecc_enabled;
 		break;
 
 	default:
@@ -3933,7 +3950,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
 	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects ||
 	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
-	    !pvt->ops->get_mc_regs) {
+	    !pvt->ops->get_mc_regs || !pvt->ops->ecc_enabled) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
@@ -4095,7 +4112,7 @@ static int probe_one_instance(unsigned int nid)
 		goto err_enable;
 	}
 
-	if (!ecc_enabled(pvt)) {
+	if (!pvt->ops->ecc_enabled(pvt)) {
 		ret = -ENODEV;
 
 		if (!ecc_enable_override)
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 1b6df33bb0a8..6cc3fc943fcd 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -469,6 +469,7 @@ struct low_ops {
 	void (*determine_memory_type)(struct amd64_pvt *pvt);
 	void (*determine_ecc_sym_sz)(struct amd64_pvt *pvt);
 	void (*get_mc_regs)(struct amd64_pvt *pvt);
+	bool (*ecc_enabled)(struct amd64_pvt *pvt);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 07/14] EDAC/amd64: Add ecc_enabled() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:47PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for ecc_enabled() in pvt->ops and assign
> family specific ecc_enabled() definitions appropriately.
>

Please include the "why".
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 77 ++++++++++++++++++++++++---------------
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 48 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 713ffe763e64..15d775a9ce7e 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3649,49 +3649,60 @@ static void restore_ecc_error_reporting(struct ecc_settings *s, u16 nid,
>  		amd64_warn("Error restoring NB MCGCTL settings!\n");
>  }
>  
> -static bool ecc_enabled(struct amd64_pvt *pvt)
> +static bool f1x_ecc_enabled(struct amd64_pvt *pvt)
>  {
>  	u16 nid = pvt->mc_node_id;
>  	bool nb_mce_en = false;
> -	u8 ecc_en = 0, i;
> +	u8 ecc_en = 0;
>  	u32 value;
>  
> -	if (boot_cpu_data.x86 >= 0x17) {
> -		u8 umc_en_mask = 0, ecc_en_mask = 0;
> -		struct amd64_umc *umc;
> +	amd64_read_pci_cfg(pvt->F3, NBCFG, &value);
>  
> -		for_each_umc(i) {
> -			umc = &pvt->umc[i];
> +	ecc_en = !!(value & NBCFG_ECC_ENABLE);
>  
> -			/* Only check enabled UMCs. */
> -			if (!(umc->sdp_ctrl & UMC_SDP_INIT))
> -				continue;
> +	nb_mce_en = nb_mce_bank_enabled_on_node(nid);
> +	if (!nb_mce_en)
> +		edac_dbg(0, "NB MCE bank disabled, set MSR 0x%08x[4] on node %d to enable.\n",
> +			 MSR_IA32_MCG_CTL, nid);
>  
> -			umc_en_mask |= BIT(i);
> +	edac_dbg(3, "Node %d: DRAM ECC %s.\n", nid, (ecc_en ? "enabled" : "disabled"));
>  
> -			if (umc->umc_cap_hi & UMC_ECC_ENABLED)
> -				ecc_en_mask |= BIT(i);
> -		}
> +	if (!ecc_en || !nb_mce_en)
> +		return false;
> +	else
> +		return true;
> +}
>  
> -		/* Check whether at least one UMC is enabled: */
> -		if (umc_en_mask)
> -			ecc_en = umc_en_mask == ecc_en_mask;
> -		else
> -			edac_dbg(0, "Node %d: No enabled UMCs.\n", nid);
> +static bool f17_ecc_enabled(struct amd64_pvt *pvt)
> +{
> +	u8 umc_en_mask = 0, ecc_en_mask = 0;
> +	u8 ecc_en = 0, i;

This line should go at the end to keep the longest->shortest line style.

> +	u16 nid = pvt->mc_node_id;
> +	bool nb_mce_en = false;
> +	struct amd64_umc *umc;
>  
> -		/* Assume UMC MCA banks are enabled. */
> -		nb_mce_en = true;
> -	} else {
> -		amd64_read_pci_cfg(pvt->F3, NBCFG, &value);
> +	for_each_umc(i) {
> +		umc = &pvt->umc[i];
> +
> +		/* Only check enabled UMCs. */
> +		if (!(umc->sdp_ctrl & UMC_SDP_INIT))
> +			continue;
>  
> -		ecc_en = !!(value & NBCFG_ECC_ENABLE);
> +		umc_en_mask |= BIT(i);
>  
> -		nb_mce_en = nb_mce_bank_enabled_on_node(nid);
> -		if (!nb_mce_en)
> -			edac_dbg(0, "NB MCE bank disabled, set MSR 0x%08x[4] on node %d to enable.\n",
> -				     MSR_IA32_MCG_CTL, nid);
> +		if (umc->umc_cap_hi & UMC_ECC_ENABLED)
> +			ecc_en_mask |= BIT(i);
>  	}
>  
> +	/* Check whether at least one UMC is enabled: */
> +	if (umc_en_mask)
> +		ecc_en = umc_en_mask == ecc_en_mask;
> +	else
> +		edac_dbg(0, "Node %d: No enabled UMCs.\n", nid);
> +
> +	/* Assume UMC MCA banks are enabled. */
> +	nb_mce_en = true;
> +

The nb_mce_en variable can be dropped since this is now a separate function.

Thanks,
Yazen

[PATCH 08/14] EDAC/amd64: Add determine_edac_cap() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for determine_edac_cap() in pvt->ops and assign
family specific determine_edac_cap() definitions appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 37 ++++++++++++++++++++++++-------------
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 15d775a9ce7e..af6711cf03e9 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1267,13 +1267,25 @@ static int get_channel_from_ecc_syndrome(struct mem_ctl_info *, u16);
  * Determine if the DIMMs have ECC enabled. ECC is enabled ONLY if all the DIMMs
  * are ECC capable.
  */
-static unsigned long determine_edac_cap(struct amd64_pvt *pvt)
+static unsigned long f1x_determine_edac_cap(struct amd64_pvt *pvt)
 {
 	unsigned long edac_cap = EDAC_FLAG_NONE;
 	u8 bit;
 
-	if (pvt->umc) {
-		u8 i, umc_en_mask = 0, dimm_ecc_en_mask = 0;
+	bit = (pvt->fam > 0xf || pvt->ext_model >= K8_REV_F)
+		? 19
+		: 17;
+
+	if (pvt->dclr0 & BIT(bit))
+		edac_cap = EDAC_FLAG_SECDED;
+
+	return edac_cap;
+}
+
+static unsigned long f17_determine_edac_cap(struct amd64_pvt *pvt)
+{
+	u8 i, umc_en_mask = 0, dimm_ecc_en_mask = 0;
+	unsigned long edac_cap = EDAC_FLAG_NONE;
 
 		for_each_umc(i) {
 			if (!(pvt->umc[i].sdp_ctrl & UMC_SDP_INIT))
@@ -1288,14 +1300,6 @@ static unsigned long determine_edac_cap(struct amd64_pvt *pvt)
 
 		if (umc_en_mask == dimm_ecc_en_mask)
 			edac_cap = EDAC_FLAG_SECDED;
-	} else {
-		bit = (pvt->fam > 0xf || pvt->ext_model >= K8_REV_F)
-			? 19
-			: 17;
-
-		if (pvt->dclr0 & BIT(bit))
-			edac_cap = EDAC_FLAG_SECDED;
-	}
 
 	return edac_cap;
 }
@@ -3759,7 +3763,7 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci)
 			mci->edac_ctl_cap |= EDAC_FLAG_S4ECD4ED;
 	}
 
-	mci->edac_cap		= determine_edac_cap(pvt);
+	mci->edac_cap		= pvt->ops->determine_edac_cap(pvt);
 	mci->mod_name		= EDAC_MOD_STR;
 	mci->ctl_name		= pvt->ctl_name;
 	mci->dev_name		= pci_name(pvt->F3);
@@ -3796,6 +3800,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= read_mc_regs;
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
+		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		break;
 
 	case 0x10:
@@ -3811,6 +3816,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= read_mc_regs;
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
+		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		break;
 
 	case 0x15:
@@ -3842,6 +3848,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= read_mc_regs;
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
+		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		break;
 
 	case 0x16:
@@ -3863,6 +3870,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= read_mc_regs;
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
+		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		break;
 
 	case 0x17:
@@ -3898,6 +3906,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= __read_mc_regs_df;
 		pvt->ops->ecc_enabled			= f17_ecc_enabled;
+		pvt->ops->determine_edac_cap		= f17_determine_edac_cap;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3939,6 +3948,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
 		pvt->ops->get_mc_regs			= __read_mc_regs_df;
 		pvt->ops->ecc_enabled			= f17_ecc_enabled;
+		pvt->ops->determine_edac_cap		= f17_determine_edac_cap;
 		break;
 
 	default:
@@ -3950,7 +3960,8 @@ static int per_family_init(struct amd64_pvt *pvt)
 	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
 	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects ||
 	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
-	    !pvt->ops->get_mc_regs || !pvt->ops->ecc_enabled) {
+	    !pvt->ops->get_mc_regs || !pvt->ops->ecc_enabled ||
+	    !pvt->ops->determine_edac_cap) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 6cc3fc943fcd..9a789cb01f4d 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -470,6 +470,7 @@ struct low_ops {
 	void (*determine_ecc_sym_sz)(struct amd64_pvt *pvt);
 	void (*get_mc_regs)(struct amd64_pvt *pvt);
 	bool (*ecc_enabled)(struct amd64_pvt *pvt);
+	unsigned long (*determine_edac_cap)(struct amd64_pvt *pvt);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 08/14] EDAC/amd64: Add determine_edac_cap() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:48PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for determine_edac_cap() in pvt->ops and assign
> family specific determine_edac_cap() definitions appropriately.
>

Please include the "why".
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 37 ++++++++++++++++++++++++-------------
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 15d775a9ce7e..af6711cf03e9 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1267,13 +1267,25 @@ static int get_channel_from_ecc_syndrome(struct mem_ctl_info *, u16);
>   * Determine if the DIMMs have ECC enabled. ECC is enabled ONLY if all the DIMMs
>   * are ECC capable.
>   */
> -static unsigned long determine_edac_cap(struct amd64_pvt *pvt)
> +static unsigned long f1x_determine_edac_cap(struct amd64_pvt *pvt)
>  {
>  	unsigned long edac_cap = EDAC_FLAG_NONE;
>  	u8 bit;
>  
> -	if (pvt->umc) {
> -		u8 i, umc_en_mask = 0, dimm_ecc_en_mask = 0;
> +	bit = (pvt->fam > 0xf || pvt->ext_model >= K8_REV_F)
> +		? 19
> +		: 17;
> +
> +	if (pvt->dclr0 & BIT(bit))
> +		edac_cap = EDAC_FLAG_SECDED;
> +
> +	return edac_cap;
> +}
> +
> +static unsigned long f17_determine_edac_cap(struct amd64_pvt *pvt)
> +{
> +	u8 i, umc_en_mask = 0, dimm_ecc_en_mask = 0;
> +	unsigned long edac_cap = EDAC_FLAG_NONE;
>  
>  		for_each_umc(i) {
>  			if (!(pvt->umc[i].sdp_ctrl & UMC_SDP_INIT))
> @@ -1288,14 +1300,6 @@ static unsigned long determine_edac_cap(struct amd64_pvt *pvt)
>  
>  		if (umc_en_mask == dimm_ecc_en_mask)
>  			edac_cap = EDAC_FLAG_SECDED;
> -	} else {
> -		bit = (pvt->fam > 0xf || pvt->ext_model >= K8_REV_F)
> -			? 19
> -			: 17;
> -
> -		if (pvt->dclr0 & BIT(bit))
> -			edac_cap = EDAC_FLAG_SECDED;
> -	}
>  
>  	return edac_cap;
>  }
> @@ -3759,7 +3763,7 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci)
>  			mci->edac_ctl_cap |= EDAC_FLAG_S4ECD4ED;

This patch is okay overall. But I think we should audit which EDAC_FLAG values
are used to make sure they truly match the system capabilities.

Thanks,
Yazen

[PATCH 09/14] EDAC/amd64: Add determine_edac_ctl_cap() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for determine_edac_ctl_cap() in pvt->ops and assign
family specific determine_edac_ctl_cap() definitions appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 30 +++++++++++++++++++-----------
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index af6711cf03e9..e3b0a0329f43 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3716,7 +3716,17 @@ static bool f17_ecc_enabled(struct amd64_pvt *pvt)
 }
 
 static inline void
-f17h_determine_edac_ctl_cap(struct mem_ctl_info *mci, struct amd64_pvt *pvt)
+f1x_determine_edac_ctl_cap(struct mem_ctl_info *mci, struct amd64_pvt *pvt)
+{
+	if (pvt->nbcap & NBCAP_SECDED)
+		mci->edac_ctl_cap |= EDAC_FLAG_SECDED;
+
+	if (pvt->nbcap & NBCAP_CHIPKILL)
+		mci->edac_ctl_cap |= EDAC_FLAG_S4ECD4ED;
+}
+
+static inline void
+f17_determine_edac_ctl_cap(struct mem_ctl_info *mci, struct amd64_pvt *pvt)
 {
 	u8 i, ecc_en = 1, cpk_en = 1, dev_x4 = 1, dev_x16 = 1;
 
@@ -3753,15 +3763,7 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci)
 	mci->mtype_cap		= MEM_FLAG_DDR2 | MEM_FLAG_RDDR2;
 	mci->edac_ctl_cap	= EDAC_FLAG_NONE;
 
-	if (pvt->umc) {
-		f17h_determine_edac_ctl_cap(mci, pvt);
-	} else {
-		if (pvt->nbcap & NBCAP_SECDED)
-			mci->edac_ctl_cap |= EDAC_FLAG_SECDED;
-
-		if (pvt->nbcap & NBCAP_CHIPKILL)
-			mci->edac_ctl_cap |= EDAC_FLAG_S4ECD4ED;
-	}
+	pvt->ops->determine_edac_ctl_cap(mci, pvt);
 
 	mci->edac_cap		= pvt->ops->determine_edac_cap(pvt);
 	mci->mod_name		= EDAC_MOD_STR;
@@ -3801,6 +3803,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_mc_regs			= read_mc_regs;
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
+		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		break;
 
 	case 0x10:
@@ -3817,6 +3820,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_mc_regs			= read_mc_regs;
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
+		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		break;
 
 	case 0x15:
@@ -3849,6 +3853,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_mc_regs			= read_mc_regs;
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
+		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		break;
 
 	case 0x16:
@@ -3871,6 +3876,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_mc_regs			= read_mc_regs;
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
+		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		break;
 
 	case 0x17:
@@ -3907,6 +3913,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_mc_regs			= __read_mc_regs_df;
 		pvt->ops->ecc_enabled			= f17_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f17_determine_edac_cap;
+		pvt->ops->determine_edac_ctl_cap	= f17_determine_edac_ctl_cap;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3949,6 +3956,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->get_mc_regs			= __read_mc_regs_df;
 		pvt->ops->ecc_enabled			= f17_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f17_determine_edac_cap;
+		pvt->ops->determine_edac_ctl_cap	= f17_determine_edac_ctl_cap;
 		break;
 
 	default:
@@ -3961,7 +3969,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects ||
 	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
 	    !pvt->ops->get_mc_regs || !pvt->ops->ecc_enabled ||
-	    !pvt->ops->determine_edac_cap) {
+	    !pvt->ops->determine_edac_cap || !pvt->ops->determine_edac_ctl_cap) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 9a789cb01f4d..0e0715a16981 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -471,6 +471,7 @@ struct low_ops {
 	void (*get_mc_regs)(struct amd64_pvt *pvt);
 	bool (*ecc_enabled)(struct amd64_pvt *pvt);
 	unsigned long (*determine_edac_cap)(struct amd64_pvt *pvt);
+	void (*determine_edac_ctl_cap)(struct mem_ctl_info *mci, struct amd64_pvt *pvt);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 09/14] EDAC/amd64: Add determine_edac_ctl_cap() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:49PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for determine_edac_ctl_cap() in pvt->ops and assign
> family specific determine_edac_ctl_cap() definitions appropriately.
>

Please include the "why".

And like the previous patch, this looks okay overall but we should verify the
EDAC_FLAGs.

Thanks,
Yazen 

[PATCH 10/14] EDAC/amd64: Add setup_mci_misc_sttrs() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for setup_mci_misc_sttrs() in pvt->ops and assign
family specific setup_mci_misc_sttrs() definitions appropriately

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 13 ++++++++++---
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index e3b0a0329f43..c86674c3238d 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3756,7 +3756,7 @@ f17_determine_edac_ctl_cap(struct mem_ctl_info *mci, struct amd64_pvt *pvt)
 	}
 }
 
-static void setup_mci_misc_attrs(struct mem_ctl_info *mci)
+static void f1x_setup_mci_misc_attrs(struct mem_ctl_info *mci)
 {
 	struct amd64_pvt *pvt = mci->pvt_info;
 
@@ -3804,6 +3804,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
+		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		break;
 
 	case 0x10:
@@ -3821,6 +3822,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
+		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		break;
 
 	case 0x15:
@@ -3854,6 +3856,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
+		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		break;
 
 	case 0x16:
@@ -3877,6 +3880,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->ecc_enabled			= f1x_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
+		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		break;
 
 	case 0x17:
@@ -3914,6 +3918,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->ecc_enabled			= f17_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f17_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f17_determine_edac_ctl_cap;
+		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3957,6 +3962,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->ecc_enabled			= f17_ecc_enabled;
 		pvt->ops->determine_edac_cap		= f17_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f17_determine_edac_ctl_cap;
+		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		break;
 
 	default:
@@ -3969,7 +3975,8 @@ static int per_family_init(struct amd64_pvt *pvt)
 	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects ||
 	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
 	    !pvt->ops->get_mc_regs || !pvt->ops->ecc_enabled ||
-	    !pvt->ops->determine_edac_cap || !pvt->ops->determine_edac_ctl_cap) {
+	    !pvt->ops->determine_edac_cap || !pvt->ops->determine_edac_ctl_cap ||
+	    !pvt->ops->setup_mci_misc_attrs) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
@@ -4064,7 +4071,7 @@ static int init_one_instance(struct amd64_pvt *pvt)
 	mci->pvt_info = pvt;
 	mci->pdev = &pvt->F3->dev;
 
-	setup_mci_misc_attrs(mci);
+	pvt->ops->setup_mci_misc_attrs(mci);
 
 	if (init_csrows(mci))
 		mci->edac_cap = EDAC_FLAG_NONE;
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 0e0715a16981..1ffee0009a53 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -472,6 +472,7 @@ struct low_ops {
 	bool (*ecc_enabled)(struct amd64_pvt *pvt);
 	unsigned long (*determine_edac_cap)(struct amd64_pvt *pvt);
 	void (*determine_edac_ctl_cap)(struct mem_ctl_info *mci, struct amd64_pvt *pvt);
+	void (*setup_mci_misc_attrs)(struct mem_ctl_info *mci);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 10/14] EDAC/amd64: Add setup_mci_misc_sttrs() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:50PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for setup_mci_misc_sttrs() in pvt->ops and assign
> family specific setup_mci_misc_sttrs() definitions appropriately
>

Please include the "why".
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 13 ++++++++++---
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index e3b0a0329f43..c86674c3238d 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3756,7 +3756,7 @@ f17_determine_edac_ctl_cap(struct mem_ctl_info *mci, struct amd64_pvt *pvt)
>  	}
>  }
>  
> -static void setup_mci_misc_attrs(struct mem_ctl_info *mci)
> +static void f1x_setup_mci_misc_attrs(struct mem_ctl_info *mci)

The MEM_FLAG values aren't correct in this function. I'd like to say we can
just not set mtype_cap since it's only used in a single debug print statement.
But that's in edac_mc, so maybe some users expect it?

Thanks,
Yazen

[PATCH 11/14] EDAC/amd64: Add populate_csrows() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for populate_csrows() in pvt->ops and assign
family specific populate_csrows() definitions appropriately

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 13 ++++++++-----
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index c86674c3238d..a799594c9574 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3430,9 +3430,6 @@ static int init_csrows(struct mem_ctl_info *mci)
 	int nr_pages = 0;
 	u32 val;
 
-	if (pvt->umc)
-		return init_csrows_df(mci);
-
 	amd64_read_pci_cfg(pvt->F3, NBCFG, &val);
 
 	pvt->nbcfg = val;
@@ -3805,6 +3802,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
+		pvt->ops->populate_csrows		= init_csrows;
 		break;
 
 	case 0x10:
@@ -3823,6 +3821,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
+		pvt->ops->populate_csrows		= init_csrows;
 		break;
 
 	case 0x15:
@@ -3857,6 +3856,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
+		pvt->ops->populate_csrows		= init_csrows;
 		break;
 
 	case 0x16:
@@ -3881,6 +3881,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_cap		= f1x_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
+		pvt->ops->populate_csrows		= init_csrows;
 		break;
 
 	case 0x17:
@@ -3919,6 +3920,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_cap		= f17_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f17_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
+		pvt->ops->populate_csrows		= init_csrows_df;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3963,6 +3965,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_cap		= f17_determine_edac_cap;
 		pvt->ops->determine_edac_ctl_cap	= f17_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
+		pvt->ops->populate_csrows		= init_csrows_df;
 		break;
 
 	default:
@@ -3976,7 +3979,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
 	    !pvt->ops->get_mc_regs || !pvt->ops->ecc_enabled ||
 	    !pvt->ops->determine_edac_cap || !pvt->ops->determine_edac_ctl_cap ||
-	    !pvt->ops->setup_mci_misc_attrs) {
+	    !pvt->ops->setup_mci_misc_attrs || !pvt->ops->populate_csrows) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
@@ -4073,7 +4076,7 @@ static int init_one_instance(struct amd64_pvt *pvt)
 
 	pvt->ops->setup_mci_misc_attrs(mci);
 
-	if (init_csrows(mci))
+	if (pvt->ops->populate_csrows(mci))
 		mci->edac_cap = EDAC_FLAG_NONE;
 
 	ret = -ENODEV;
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 1ffee0009a53..c762b341650f 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -473,6 +473,7 @@ struct low_ops {
 	unsigned long (*determine_edac_cap)(struct amd64_pvt *pvt);
 	void (*determine_edac_ctl_cap)(struct mem_ctl_info *mci, struct amd64_pvt *pvt);
 	void (*setup_mci_misc_attrs)(struct mem_ctl_info *mci);
+	int  (*populate_csrows)(struct mem_ctl_info *mci);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 11/14] EDAC/amd64: Add populate_csrows() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:51PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for populate_csrows() in pvt->ops and assign
> family specific populate_csrows() definitions appropriately
>

Please include the "why", but otherwise looks okay.

Thanks,
Yazen 

[PATCH 12/14] EDAC/amd64: Add dump_misc_regs() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for dump_misc_regs() in pvt->ops and assign
family specific dump_misc_regs() definitions appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 25 ++++++++++++++-----------
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index a799594c9574..1063dda20ce9 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1442,6 +1442,10 @@ static void __dump_misc_regs_df(struct amd64_pvt *pvt)
 
 	edac_dbg(1, "F0x104 (DRAM Hole Address): 0x%08x, base: 0x%08x\n",
 		 pvt->dhar, dhar_base(pvt));
+
+	edac_dbg(1, "  DramHoleValid: %s\n", dhar_valid(pvt) ? "yes" : "no");
+
+	amd64_info("using x%u syndromes.\n", pvt->ecc_sym_sz);
 }
 
 /* Display and decode various NB registers for debug purposes. */
@@ -1476,15 +1480,6 @@ static void __dump_misc_regs(struct amd64_pvt *pvt)
 	/* Only if NOT ganged does dclr1 have valid info */
 	if (!dct_ganging_enabled(pvt))
 		debug_dump_dramcfg_low(pvt, pvt->dclr1, 1);
-}
-
-/* Display and decode various NB registers for debug purposes. */
-static void dump_misc_regs(struct amd64_pvt *pvt)
-{
-	if (pvt->umc)
-		__dump_misc_regs_df(pvt);
-	else
-		__dump_misc_regs(pvt);
 
 	edac_dbg(1, "  DramHoleValid: %s\n", dhar_valid(pvt) ? "yes" : "no");
 
@@ -3803,6 +3798,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows;
+		pvt->ops->dump_misc_regs		= __dump_misc_regs;
 		break;
 
 	case 0x10:
@@ -3822,6 +3818,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows;
+		pvt->ops->dump_misc_regs		= __dump_misc_regs;
 		break;
 
 	case 0x15:
@@ -3857,6 +3854,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows;
+		pvt->ops->dump_misc_regs		= __dump_misc_regs;
 		break;
 
 	case 0x16:
@@ -3882,6 +3880,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows;
+		pvt->ops->dump_misc_regs		= __dump_misc_regs;
 		break;
 
 	case 0x17:
@@ -3921,6 +3920,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_ctl_cap	= f17_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows_df;
+		pvt->ops->dump_misc_regs		= __dump_misc_regs_df;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3966,6 +3966,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->determine_edac_ctl_cap	= f17_determine_edac_ctl_cap;
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows_df;
+		pvt->ops->dump_misc_regs		= __dump_misc_regs_df;
 		break;
 
 	default:
@@ -3979,7 +3980,8 @@ static int per_family_init(struct amd64_pvt *pvt)
 	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
 	    !pvt->ops->get_mc_regs || !pvt->ops->ecc_enabled ||
 	    !pvt->ops->determine_edac_cap || !pvt->ops->determine_edac_ctl_cap ||
-	    !pvt->ops->setup_mci_misc_attrs || !pvt->ops->populate_csrows) {
+	    !pvt->ops->setup_mci_misc_attrs || !pvt->ops->populate_csrows ||
+	    !pvt->ops->dump_misc_regs) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
@@ -4169,7 +4171,8 @@ static int probe_one_instance(unsigned int nid)
 
 	amd64_info("%s detected (node %d).\n", pvt->ctl_name, pvt->mc_node_id);
 
-	dump_misc_regs(pvt);
+	/* Display and decode various NB registers for debug purposes. */
+	pvt->ops->dump_misc_regs(pvt);
 
 	return ret;
 
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index c762b341650f..7b377dba0dc7 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -474,6 +474,7 @@ struct low_ops {
 	void (*determine_edac_ctl_cap)(struct mem_ctl_info *mci, struct amd64_pvt *pvt);
 	void (*setup_mci_misc_attrs)(struct mem_ctl_info *mci);
 	int  (*populate_csrows)(struct mem_ctl_info *mci);
+	void (*dump_misc_regs)(struct amd64_pvt *pvt);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 12/14] EDAC/amd64: Add dump_misc_regs() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:52PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for dump_misc_regs() in pvt->ops and assign
> family specific dump_misc_regs() definitions appropriately.

Please include the "why".

> 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 25 ++++++++++++++-----------
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index a799594c9574..1063dda20ce9 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1442,6 +1442,10 @@ static void __dump_misc_regs_df(struct amd64_pvt *pvt)
>  
>  	edac_dbg(1, "F0x104 (DRAM Hole Address): 0x%08x, base: 0x%08x\n",
>  		 pvt->dhar, dhar_base(pvt));
> +
> +	edac_dbg(1, "  DramHoleValid: %s\n", dhar_valid(pvt) ? "yes" : "no");
> +

This line can be dropped when dhar is removed for current systems.

> +	amd64_info("using x%u syndromes.\n", pvt->ecc_sym_sz);

This line can be dropped if we decide that symbol size isn't needed on current
systems. We should double check if it's needed when determining "edac_mode".

If we keep it, then it should be printed at the end of the function where we
read the symbol size.

>  }
>  
>  /* Display and decode various NB registers for debug purposes. */
> @@ -1476,15 +1480,6 @@ static void __dump_misc_regs(struct amd64_pvt *pvt)
>  	/* Only if NOT ganged does dclr1 have valid info */
>  	if (!dct_ganging_enabled(pvt))
>  		debug_dump_dramcfg_low(pvt, pvt->dclr1, 1);
> -}
> -
> -/* Display and decode various NB registers for debug purposes. */
> -static void dump_misc_regs(struct amd64_pvt *pvt)
> -{
> -	if (pvt->umc)
> -		__dump_misc_regs_df(pvt);
> -	else
> -		__dump_misc_regs(pvt);
>  
>  	edac_dbg(1, "  DramHoleValid: %s\n", dhar_valid(pvt) ? "yes" : "no");
>  
> @@ -3803,6 +3798,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
>  		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
>  		pvt->ops->populate_csrows		= init_csrows;
> +		pvt->ops->dump_misc_regs		= __dump_misc_regs;
>  		break;
>  
>  	case 0x10:
> @@ -3822,6 +3818,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
>  		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
>  		pvt->ops->populate_csrows		= init_csrows;
> +		pvt->ops->dump_misc_regs		= __dump_misc_regs;
>  		break;
>  
>  	case 0x15:
> @@ -3857,6 +3854,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
>  		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
>  		pvt->ops->populate_csrows		= init_csrows;
> +		pvt->ops->dump_misc_regs		= __dump_misc_regs;
>  		break;
>  
>  	case 0x16:
> @@ -3882,6 +3880,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->determine_edac_ctl_cap	= f1x_determine_edac_ctl_cap;
>  		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
>  		pvt->ops->populate_csrows		= init_csrows;
> +		pvt->ops->dump_misc_regs		= __dump_misc_regs;
>  		break;
>  
>  	case 0x17:
> @@ -3921,6 +3920,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->determine_edac_ctl_cap	= f17_determine_edac_ctl_cap;
>  		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
>  		pvt->ops->populate_csrows		= init_csrows_df;
> +		pvt->ops->dump_misc_regs		= __dump_misc_regs_df;
>  
>  		if (pvt->fam == 0x18) {
>  			pvt->ctl_name			= "F18h";
> @@ -3966,6 +3966,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->determine_edac_ctl_cap	= f17_determine_edac_ctl_cap;
>  		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
>  		pvt->ops->populate_csrows		= init_csrows_df;
> +		pvt->ops->dump_misc_regs		= __dump_misc_regs_df;

I think the underscore prefixes can be dropped.

>  		break;
>  
>  	default:
> @@ -3979,7 +3980,8 @@ static int per_family_init(struct amd64_pvt *pvt)
>  	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
>  	    !pvt->ops->get_mc_regs || !pvt->ops->ecc_enabled ||
>  	    !pvt->ops->determine_edac_cap || !pvt->ops->determine_edac_ctl_cap ||
> -	    !pvt->ops->setup_mci_misc_attrs || !pvt->ops->populate_csrows) {
> +	    !pvt->ops->setup_mci_misc_attrs || !pvt->ops->populate_csrows ||
> +	    !pvt->ops->dump_misc_regs) {
>  		edac_dbg(1, "Common helper routines not defined.\n");
>  		return -EFAULT;
>  	}
> @@ -4169,7 +4171,8 @@ static int probe_one_instance(unsigned int nid)
>  
>  	amd64_info("%s detected (node %d).\n", pvt->ctl_name, pvt->mc_node_id);
>  
> -	dump_misc_regs(pvt);
> +	/* Display and decode various NB registers for debug purposes. */

Please drop "NB" from the comment since other registers can also be printed.

Thanks,
Yazen

[PATCH 13/14] EDAC/amd64: Add get_cs_mode() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for get_cs_mode() in pvt->ops and assign
family specific get_cs_mode() definitions appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 23 +++++++++++++++--------
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 1063dda20ce9..7a20f8a696de 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1347,6 +1347,13 @@ static void debug_dump_dramcfg_low(struct amd64_pvt *pvt, u32 dclr, int chan)
 #define CS_EVEN			(CS_EVEN_PRIMARY | CS_EVEN_SECONDARY)
 #define CS_ODD			(CS_ODD_PRIMARY | CS_ODD_SECONDARY)
 
+static int f1x_get_cs_mode(int dimm, u8 ctrl, struct amd64_pvt *pvt)
+{
+	u32 dbam = ctrl ? pvt->dbam1 : pvt->dbam0;
+
+	return DBAM_DIMM(dimm, dbam);
+}
+
 static int f17_get_cs_mode(int dimm, u8 ctrl, struct amd64_pvt *pvt)
 {
 	u8 base, count = 0;
@@ -3346,16 +3353,10 @@ static void read_mc_regs(struct amd64_pvt *pvt)
  */
 static u32 get_csrow_nr_pages(struct amd64_pvt *pvt, u8 dct, int csrow_nr_orig)
 {
-	u32 dbam = dct ? pvt->dbam1 : pvt->dbam0;
 	int csrow_nr = csrow_nr_orig;
 	u32 cs_mode, nr_pages;
 
-	if (!pvt->umc) {
-		csrow_nr >>= 1;
-		cs_mode = DBAM_DIMM(csrow_nr, dbam);
-	} else {
-		cs_mode = f17_get_cs_mode(csrow_nr >> 1, dct, pvt);
-	}
+	cs_mode = pvt->ops->get_cs_mode(csrow_nr >> 1, dct, pvt);
 
 	nr_pages   = pvt->ops->dbam_to_cs(pvt, dct, cs_mode, csrow_nr);
 	nr_pages <<= 20 - PAGE_SHIFT;
@@ -3799,6 +3800,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows;
 		pvt->ops->dump_misc_regs		= __dump_misc_regs;
+		pvt->ops->get_cs_mode			= f1x_get_cs_mode;
 		break;
 
 	case 0x10:
@@ -3819,6 +3821,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows;
 		pvt->ops->dump_misc_regs		= __dump_misc_regs;
+		pvt->ops->get_cs_mode			= f1x_get_cs_mode;
 		break;
 
 	case 0x15:
@@ -3855,6 +3858,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows;
 		pvt->ops->dump_misc_regs		= __dump_misc_regs;
+		pvt->ops->get_cs_mode			= f1x_get_cs_mode;
 		break;
 
 	case 0x16:
@@ -3881,6 +3885,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows;
 		pvt->ops->dump_misc_regs		= __dump_misc_regs;
+		pvt->ops->get_cs_mode			= f1x_get_cs_mode;
 		break;
 
 	case 0x17:
@@ -3921,6 +3926,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows_df;
 		pvt->ops->dump_misc_regs		= __dump_misc_regs_df;
+		pvt->ops->get_cs_mode			= f17_get_cs_mode;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3967,6 +3973,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->setup_mci_misc_attrs		= f1x_setup_mci_misc_attrs;
 		pvt->ops->populate_csrows		= init_csrows_df;
 		pvt->ops->dump_misc_regs		= __dump_misc_regs_df;
+		pvt->ops->get_cs_mode			= f17_get_cs_mode;
 		break;
 
 	default:
@@ -3981,7 +3988,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 	    !pvt->ops->get_mc_regs || !pvt->ops->ecc_enabled ||
 	    !pvt->ops->determine_edac_cap || !pvt->ops->determine_edac_ctl_cap ||
 	    !pvt->ops->setup_mci_misc_attrs || !pvt->ops->populate_csrows ||
-	    !pvt->ops->dump_misc_regs) {
+	    !pvt->ops->dump_misc_regs || !pvt->ops->get_cs_mode) {
 		edac_dbg(1, "Common helper routines not defined.\n");
 		return -EFAULT;
 	}
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 7b377dba0dc7..2c93f8e0021a 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -475,6 +475,7 @@ struct low_ops {
 	void (*setup_mci_misc_attrs)(struct mem_ctl_info *mci);
 	int  (*populate_csrows)(struct mem_ctl_info *mci);
 	void (*dump_misc_regs)(struct amd64_pvt *pvt);
+	int  (*get_cs_mode)(int dimm, u8 ctrl, struct amd64_pvt *pvt);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 13/14] EDAC/amd64: Add get_cs_mode() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:53PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for get_cs_mode() in pvt->ops and assign
> family specific get_cs_mode() definitions appropriately.
>

Please include the "why", but otherwise this seems okay.

Thanks,
Yazen

[PATCH 14/14] EDAC/amd64: Add get_umc_error_info() into pvt->ops

[Naveen Krishna Chatradhi]

From: Muralidhara M K <muralimk@amd.com>

Add function pointer for get_umc_error_info() in pvt->ops and assign
family specific get_umc_error_info() definitions appropriately.

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
This patch is created by splitting the 5/12th patch in series
[v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/

 drivers/edac/amd64_edac.c | 19 ++++++++++++++-----
 drivers/edac/amd64_edac.h |  1 +
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 7a20f8a696de..ab4e16070a02 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3056,10 +3056,13 @@ static inline void decode_bus_error(int node_id, struct mce *m)
  * Currently, we can derive the channel number by looking at the 6th nibble in
  * the instance_id. For example, instance_id=0xYXXXXX where Y is the channel
  * number.
+ *
+ * csrow can be derived from the lower 3 bits of MCA_SYND value.
  */
-static int find_umc_channel(struct mce *m)
+static void f17_umc_err_info(struct mce *m, struct err_info *err)
 {
-	return (m->ipid & GENMASK(31, 0)) >> 20;
+	err->channel = (m->ipid & GENMASK(31, 0)) >> 20;
+	err->csrow = m->synd & 0x7;
 }
 
 static void decode_umc_error(int node_id, struct mce *m)
@@ -3081,8 +3084,6 @@ static void decode_umc_error(int node_id, struct mce *m)
 	if (m->status & MCI_STATUS_DEFERRED)
 		ecc_type = 3;
 
-	err.channel = find_umc_channel(m);
-
 	if (!(m->status & MCI_STATUS_SYNDV)) {
 		err.err_code = ERR_SYND;
 		goto log_error;
@@ -3097,7 +3098,7 @@ static void decode_umc_error(int node_id, struct mce *m)
 			err.err_code = ERR_CHANNEL;
 	}
 
-	err.csrow = m->synd & 0x7;
+	pvt->ops->get_umc_error_info(m, &err);
 
 	if (umc_normaddr_to_sysaddr(m->addr, pvt->mc_node_id, err.channel, &sys_addr)) {
 		err.err_code = ERR_NORM_ADDR;
@@ -3927,6 +3928,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->populate_csrows		= init_csrows_df;
 		pvt->ops->dump_misc_regs		= __dump_misc_regs_df;
 		pvt->ops->get_cs_mode			= f17_get_cs_mode;
+		pvt->ops->get_umc_error_info		= f17_umc_err_info;
 
 		if (pvt->fam == 0x18) {
 			pvt->ctl_name			= "F18h";
@@ -3974,6 +3976,7 @@ static int per_family_init(struct amd64_pvt *pvt)
 		pvt->ops->populate_csrows		= init_csrows_df;
 		pvt->ops->dump_misc_regs		= __dump_misc_regs_df;
 		pvt->ops->get_cs_mode			= f17_get_cs_mode;
+		pvt->ops->get_umc_error_info		= f17_umc_err_info;
 		break;
 
 	default:
@@ -3993,6 +3996,12 @@ static int per_family_init(struct amd64_pvt *pvt)
 		return -EFAULT;
 	}
 
+	/* ops required for families 17h and later */
+	if (pvt->fam >= 0x17 && !pvt->ops->get_umc_error_info) {
+		edac_dbg(1, "Platform specific helper routines not defined.\n");
+		return -EFAULT;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 2c93f8e0021a..43d9b11b826d 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -476,6 +476,7 @@ struct low_ops {
 	int  (*populate_csrows)(struct mem_ctl_info *mci);
 	void (*dump_misc_regs)(struct amd64_pvt *pvt);
 	int  (*get_cs_mode)(int dimm, u8 ctrl, struct amd64_pvt *pvt);
+	void (*get_umc_error_info)(struct mce *m, struct err_info *err);
 };
 
 int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,
-- 
2.25.1

Re: [PATCH 14/14] EDAC/amd64: Add get_umc_error_info() into pvt->ops

[Yazen Ghannam]

On Mon, Feb 28, 2022 at 09:43:54PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for get_umc_error_info() in pvt->ops and assign
> family specific get_umc_error_info() definitions appropriately.
>

Please include the "why".
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 19 ++++++++++++++-----
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 7a20f8a696de..ab4e16070a02 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3056,10 +3056,13 @@ static inline void decode_bus_error(int node_id, struct mce *m)
>   * Currently, we can derive the channel number by looking at the 6th nibble in
>   * the instance_id. For example, instance_id=0xYXXXXX where Y is the channel
>   * number.
> + *
> + * csrow can be derived from the lower 3 bits of MCA_SYND value.

I think this comment can be expanded.

For DRAM ECC errors, the Chip Select number is given in bits [2:0] of
the MCA_SYND[ErrorInformation] field.

>   */
> -static int find_umc_channel(struct mce *m)
> +static void f17_umc_err_info(struct mce *m, struct err_info *err)
>  {
> -	return (m->ipid & GENMASK(31, 0)) >> 20;
> +	err->channel = (m->ipid & GENMASK(31, 0)) >> 20;
> +	err->csrow = m->synd & 0x7;
>  }
>  
>  static void decode_umc_error(int node_id, struct mce *m)
> @@ -3081,8 +3084,6 @@ static void decode_umc_error(int node_id, struct mce *m)
>  	if (m->status & MCI_STATUS_DEFERRED)
>  		ecc_type = 3;
>  
> -	err.channel = find_umc_channel(m);
> -
>  	if (!(m->status & MCI_STATUS_SYNDV)) {
>  		err.err_code = ERR_SYND;
>  		goto log_error;
> @@ -3097,7 +3098,7 @@ static void decode_umc_error(int node_id, struct mce *m)
>  			err.err_code = ERR_CHANNEL;
>  	}
>  
> -	err.csrow = m->synd & 0x7;
> +	pvt->ops->get_umc_error_info(m, &err);
>  
>  	if (umc_normaddr_to_sysaddr(m->addr, pvt->mc_node_id, err.channel, &sys_addr)) {
>  		err.err_code = ERR_NORM_ADDR;
> @@ -3927,6 +3928,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->populate_csrows		= init_csrows_df;
>  		pvt->ops->dump_misc_regs		= __dump_misc_regs_df;
>  		pvt->ops->get_cs_mode			= f17_get_cs_mode;
> +		pvt->ops->get_umc_error_info		= f17_umc_err_info;
>  
>  		if (pvt->fam == 0x18) {
>  			pvt->ctl_name			= "F18h";
> @@ -3974,6 +3976,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		pvt->ops->populate_csrows		= init_csrows_df;
>  		pvt->ops->dump_misc_regs		= __dump_misc_regs_df;
>  		pvt->ops->get_cs_mode			= f17_get_cs_mode;
> +		pvt->ops->get_umc_error_info		= f17_umc_err_info;
>  		break;
>  
>  	default:
> @@ -3993,6 +3996,12 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		return -EFAULT;
>  	}
>  
> +	/* ops required for families 17h and later */
> +	if (pvt->fam >= 0x17 && !pvt->ops->get_umc_error_info) {
> +		edac_dbg(1, "Platform specific helper routines not defined.\n");
> +		return -EFAULT;
> +	}
> +

I think this is a case where having the Family 17h+ ops as default would make
sense.

Thanks,
Yazen