linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates
@ 2022-02-02 14:43 Yazen Ghannam
  2022-02-02 14:43 ` [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM Yazen Ghannam
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Yazen Ghannam @ 2022-02-02 14:43 UTC (permalink / raw)
  To: linux-edac
  Cc: linux-kernel, bp, mchehab, tony.luck, james.morse, rric,
	Smita.KoralahalliChannabasappa, william.roche, Yazen Ghannam

Hi all,

This set adds support for AMD Family 19h Models 10h-1Fh and A0h-AFh.

Patch 1 sets the "memory type" per DIMM. 

Patch 2 adds register offset and other minor changes introduced with
these new models.

Thanks,
Yazen

Link:
https://lore.kernel.org/r/20211228200615.412999-1-yazen.ghannam@amd.com

v3->v4:
* Updated patch 1 to cache dram_type in struct umc.
* Fixed uninitiliazed variable warning in patch 2.
* Switched to a single register helper function.

v2->v3:
* Patch 1 completely reworked.
* Patch 2 updated based on comments from William.

Yazen Ghannam (2):
  EDAC/amd64: Set memory type per DIMM
  EDAC/amd64: Add new register offset support and related changes

 drivers/edac/amd64_edac.c | 113 +++++++++++++++++++++++++++++++-------
 drivers/edac/amd64_edac.h |  17 ++++++
 2 files changed, 109 insertions(+), 21 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM
  2022-02-02 14:43 [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates Yazen Ghannam
@ 2022-02-02 14:43 ` Yazen Ghannam
  2022-02-03 13:19   ` William Roche
  2022-02-23 20:55   ` Borislav Petkov
  2022-02-02 14:43 ` [PATCH v4 2/2] EDAC/amd64: Add new register offset support and related changes Yazen Ghannam
  2022-03-06 16:18 ` [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates Borislav Petkov
  2 siblings, 2 replies; 13+ messages in thread
From: Yazen Ghannam @ 2022-02-02 14:43 UTC (permalink / raw)
  To: linux-edac
  Cc: linux-kernel, bp, mchehab, tony.luck, james.morse, rric,
	Smita.KoralahalliChannabasappa, william.roche, Yazen Ghannam

Current AMD systems allow mixing of DIMM types within a system. However,
DIMMs within a channel, i.e. managed by a single Unified Memory
Controller (UMC), must be of the same type.

Handle this possible configuration by checking and setting the memory
type for each individual "UMC" structure.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lore.kernel.org/r/20211228200615.412999-2-yazen.ghannam@amd.com

v3->v4:
* Cache dram_type in struct umc.

v2->v3:
* Change patch to properly handle systems with different DIMM types.

v1->v2:
* Was patch 3 in v1.
* Update commit message.

 drivers/edac/amd64_edac.c | 47 +++++++++++++++++++++++++++++----------
 drivers/edac/amd64_edac.h |  3 +++
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index fba609ada0e6..49e384207ce0 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1429,7 +1429,7 @@ static void __dump_misc_regs_df(struct amd64_pvt *pvt)
 		edac_dbg(1, "UMC%d x16 DIMMs present: %s\n",
 				i, (umc->dimm_cfg & BIT(7)) ? "yes" : "no");
 
-		if (pvt->dram_type == MEM_LRDDR4) {
+		if (umc->dram_type == MEM_LRDDR4) {
 			amd_smn_read(pvt->mc_node_id, umc_base + UMCCH_ADDR_CFG, &tmp);
 			edac_dbg(1, "UMC%d LRDIMM %dx rank multiply\n",
 					i, 1 << ((tmp >> 4) & 0x3));
@@ -1616,19 +1616,40 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
 	}
 }
 
+static void _determine_memory_type_df(struct amd64_umc *umc)
+{
+	if (!(umc->sdp_ctrl & UMC_SDP_INIT)) {
+		umc->dram_type = MEM_EMPTY;
+		return;
+	}
+
+	if (umc->dimm_cfg & BIT(5))
+		umc->dram_type = MEM_LRDDR4;
+	else if (umc->dimm_cfg & BIT(4))
+		umc->dram_type = MEM_RDDR4;
+	else
+		umc->dram_type = MEM_DDR4;
+}
+
+static void determine_memory_type_df(struct amd64_pvt *pvt)
+{
+	struct amd64_umc *umc;
+	u32 i;
+
+	for_each_umc(i) {
+		umc = &pvt->umc[i];
+
+		_determine_memory_type_df(umc);
+		edac_dbg(1, "  UMC%d DIMM type: %s\n", i, edac_mem_types[umc->dram_type]);
+	}
+}
+
 static void 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;
-	}
+	if (pvt->umc)
+		return determine_memory_type_df(pvt);
 
 	switch (pvt->fam) {
 	case 0xf:
@@ -3452,7 +3473,9 @@ static void read_mc_regs(struct amd64_pvt *pvt)
 	read_dct_base_mask(pvt);
 
 	determine_memory_type(pvt);
-	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
+
+	if (!pvt->umc)
+		edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
 
 	determine_ecc_sym_sz(pvt);
 }
@@ -3548,7 +3571,7 @@ static int init_csrows_df(struct mem_ctl_info *mci)
 					pvt->mc_node_id, cs);
 
 			dimm->nr_pages = get_csrow_nr_pages(pvt, umc, cs);
-			dimm->mtype = pvt->dram_type;
+			dimm->mtype = pvt->umc[umc].dram_type;
 			dimm->edac_mode = edac_mode;
 			dimm->dtype = dev_type;
 			dimm->grain = 64;
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 352bda9803f6..09ad28299c57 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -344,6 +344,9 @@ struct amd64_umc {
 	u32 sdp_ctrl;		/* SDP Control reg */
 	u32 ecc_ctrl;		/* DRAM ECC Control reg */
 	u32 umc_cap_hi;		/* Capabilities High reg */
+
+	/* cache the dram_type */
+	enum mem_type dram_type;
 };
 
 struct amd64_pvt {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v4 2/2] EDAC/amd64: Add new register offset support and related changes
  2022-02-02 14:43 [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates Yazen Ghannam
  2022-02-02 14:43 ` [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM Yazen Ghannam
@ 2022-02-02 14:43 ` Yazen Ghannam
  2022-02-03 13:19   ` William Roche
  2022-03-06 16:18 ` [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates Borislav Petkov
  2 siblings, 1 reply; 13+ messages in thread
From: Yazen Ghannam @ 2022-02-02 14:43 UTC (permalink / raw)
  To: linux-edac
  Cc: linux-kernel, bp, mchehab, tony.luck, james.morse, rric,
	Smita.KoralahalliChannabasappa, william.roche, Yazen Ghannam

Introduce a "family flags" bitmask that can be used to indicate any
special behavior needed on a per-family basis.

Add a flag to indicate a system uses the new register offsets introduced
with Family 19h Model 10h.

Use this flag to account for register offset changes, a new bitfield
indicating DDR5 use on a memory controller, and to set the proper number
of chip select masks.

Rework f17_addr_mask_to_cs_size() to properly handle the change in chip
select masks. And update code comments to reflect the updated Chip
Select, DIMM, and Mask relationships.

[uninitiliazed variable warning]
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lore.kernel.org/r/20211228200615.412999-3-yazen.ghannam@amd.com

v3->v4:
* Use a single helper function for new register offsets.
* Fix an uninitialized variable warning.

v2->v3:
* Adjust variable names to explicitly show what they represent.
* Update code comment to give more detail on CS/MASK/DIMM layout.

v1->v2:
* Was patch 4 in v1.
* Change "has_ddr5" flag to "zn_regs_v2".
* Drop flag check helper function.
* Update determine_memory_type() to check bitfield for DDR5.
* Update code comments.

 drivers/edac/amd64_edac.c | 80 +++++++++++++++++++++++++++++++--------
 drivers/edac/amd64_edac.h | 14 +++++++
 2 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 49e384207ce0..5806fe657373 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -15,6 +15,21 @@ static struct msr __percpu *msrs;
 
 static struct amd64_family_type *fam_type;
 
+static inline u32 get_umc_reg(u32 reg)
+{
+	if (!fam_type->flags.zn_regs_v2)
+		return reg;
+
+	switch (reg) {
+	case UMCCH_ADDR_CFG:		return UMCCH_ADDR_CFG_DDR5;
+	case UMCCH_ADDR_MASK_SEC:	return UMCCH_ADDR_MASK_SEC_DDR5;
+	case UMCCH_DIMM_CFG:		return UMCCH_DIMM_CFG_DDR5;
+	}
+
+	WARN_ONCE(1, "%s: unknown register 0x%x", __func__, reg);
+	return 0;
+}
+
 /* Per-node stuff */
 static struct ecc_settings **ecc_stngs;
 
@@ -1429,8 +1444,10 @@ static void __dump_misc_regs_df(struct amd64_pvt *pvt)
 		edac_dbg(1, "UMC%d x16 DIMMs present: %s\n",
 				i, (umc->dimm_cfg & BIT(7)) ? "yes" : "no");
 
-		if (umc->dram_type == MEM_LRDDR4) {
-			amd_smn_read(pvt->mc_node_id, umc_base + UMCCH_ADDR_CFG, &tmp);
+		if (umc->dram_type == MEM_LRDDR4 || umc->dram_type == MEM_LRDDR5) {
+			amd_smn_read(pvt->mc_node_id,
+				     umc_base + get_umc_reg(UMCCH_ADDR_CFG),
+				     &tmp);
 			edac_dbg(1, "UMC%d LRDIMM %dx rank multiply\n",
 					i, 1 << ((tmp >> 4) & 0x3));
 		}
@@ -1505,7 +1522,7 @@ static void prep_chip_selects(struct amd64_pvt *pvt)
 
 		for_each_umc(umc) {
 			pvt->csels[umc].b_cnt = 4;
-			pvt->csels[umc].m_cnt = 2;
+			pvt->csels[umc].m_cnt = fam_type->flags.zn_regs_v2 ? 4 : 2;
 		}
 
 	} else {
@@ -1545,7 +1562,7 @@ static void read_umc_base_mask(struct amd64_pvt *pvt)
 		}
 
 		umc_mask_reg = get_umc_base(umc) + UMCCH_ADDR_MASK;
-		umc_mask_reg_sec = get_umc_base(umc) + UMCCH_ADDR_MASK_SEC;
+		umc_mask_reg_sec = get_umc_base(umc) + get_umc_reg(UMCCH_ADDR_MASK_SEC);
 
 		for_each_chip_select_mask(cs, umc, pvt) {
 			mask = &pvt->csels[umc].csmasks[cs];
@@ -1623,12 +1640,25 @@ static void _determine_memory_type_df(struct amd64_umc *umc)
 		return;
 	}
 
-	if (umc->dimm_cfg & BIT(5))
-		umc->dram_type = MEM_LRDDR4;
-	else if (umc->dimm_cfg & BIT(4))
-		umc->dram_type = MEM_RDDR4;
-	else
-		umc->dram_type = MEM_DDR4;
+	/*
+	 * Check if the system supports the "DDR Type" field in UMC Config
+	 * and has DDR5 DIMMs in use.
+	 */
+	if (fam_type->flags.zn_regs_v2 && ((umc->umc_cfg & GENMASK(2, 0)) == 0x1)) {
+		if (umc->dimm_cfg & BIT(5))
+			umc->dram_type = MEM_LRDDR5;
+		else if (umc->dimm_cfg & BIT(4))
+			umc->dram_type = MEM_RDDR5;
+		else
+			umc->dram_type = MEM_DDR5;
+	} else {
+		if (umc->dimm_cfg & BIT(5))
+			umc->dram_type = MEM_LRDDR4;
+		else if (umc->dimm_cfg & BIT(4))
+			umc->dram_type = MEM_RDDR4;
+		else
+			umc->dram_type = MEM_DDR4;
+	}
 }
 
 static void determine_memory_type_df(struct amd64_pvt *pvt)
@@ -2170,6 +2200,7 @@ static int f17_addr_mask_to_cs_size(struct amd64_pvt *pvt, u8 umc,
 {
 	u32 addr_mask_orig, addr_mask_deinterleaved;
 	u32 msb, weight, num_zero_bits;
+	int cs_mask_nr = csrow_nr;
 	int dimm, size = 0;
 
 	/* No Chip Selects are enabled. */
@@ -2185,17 +2216,33 @@ static int f17_addr_mask_to_cs_size(struct amd64_pvt *pvt, u8 umc,
 		return size;
 
 	/*
-	 * There is one mask per DIMM, and two Chip Selects per DIMM.
-	 *	CS0 and CS1 -> DIMM0
-	 *	CS2 and CS3 -> DIMM1
+	 * Family 17h introduced systems with one mask per DIMM,
+	 * and two Chip Selects per DIMM.
+	 *
+	 *	CS0 and CS1 -> MASK0 / DIMM0
+	 *	CS2 and CS3 -> MASK1 / DIMM1
+	 *
+	 * Family 19h Model 10h introduced systems with one mask per Chip Select,
+	 * and two Chip Selects per DIMM.
+	 *
+	 *	CS0 -> MASK0 -> DIMM0
+	 *	CS1 -> MASK1 -> DIMM0
+	 *	CS2 -> MASK2 -> DIMM1
+	 *	CS3 -> MASK3 -> DIMM1
+	 *
+	 * Keep the mask number equal to the Chip Select number for newer systems,
+	 * and shift the mask number for older systems.
 	 */
 	dimm = csrow_nr >> 1;
 
+	if (!fam_type->flags.zn_regs_v2)
+		cs_mask_nr >>= 1;
+
 	/* Asymmetric dual-rank DIMM support. */
 	if ((csrow_nr & 1) && (cs_mode & CS_ODD_SECONDARY))
-		addr_mask_orig = pvt->csels[umc].csmasks_sec[dimm];
+		addr_mask_orig = pvt->csels[umc].csmasks_sec[cs_mask_nr];
 	else
-		addr_mask_orig = pvt->csels[umc].csmasks[dimm];
+		addr_mask_orig = pvt->csels[umc].csmasks[cs_mask_nr];
 
 	/*
 	 * The number of zero bits in the mask is equal to the number of bits
@@ -2951,6 +2998,7 @@ static struct amd64_family_type family_types[] = {
 		.f0_id = PCI_DEVICE_ID_AMD_19H_M10H_DF_F0,
 		.f6_id = PCI_DEVICE_ID_AMD_19H_M10H_DF_F6,
 		.max_mcs = 12,
+		.flags.zn_regs_v2 = 1,
 		.ops = {
 			.early_channel_count	= f17_early_channel_count,
 			.dbam_to_cs		= f17_addr_mask_to_cs_size,
@@ -3389,7 +3437,7 @@ static void __read_mc_regs_df(struct amd64_pvt *pvt)
 		umc_base = get_umc_base(i);
 		umc = &pvt->umc[i];
 
-		amd_smn_read(nid, umc_base + UMCCH_DIMM_CFG, &umc->dimm_cfg);
+		amd_smn_read(nid, umc_base + get_umc_reg(UMCCH_DIMM_CFG), &umc->dimm_cfg);
 		amd_smn_read(nid, umc_base + UMCCH_UMC_CFG, &umc->umc_cfg);
 		amd_smn_read(nid, umc_base + UMCCH_SDP_CTRL, &umc->sdp_ctrl);
 		amd_smn_read(nid, umc_base + UMCCH_ECC_CTRL, &umc->ecc_ctrl);
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 09ad28299c57..6f8147abfa71 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -273,8 +273,11 @@
 #define UMCCH_BASE_ADDR_SEC		0x10
 #define UMCCH_ADDR_MASK			0x20
 #define UMCCH_ADDR_MASK_SEC		0x28
+#define UMCCH_ADDR_MASK_SEC_DDR5	0x30
 #define UMCCH_ADDR_CFG			0x30
+#define UMCCH_ADDR_CFG_DDR5		0x40
 #define UMCCH_DIMM_CFG			0x80
+#define UMCCH_DIMM_CFG_DDR5		0x90
 #define UMCCH_UMC_CFG			0x100
 #define UMCCH_SDP_CTRL			0x104
 #define UMCCH_ECC_CTRL			0x14C
@@ -483,11 +486,22 @@ struct low_ops {
 					 unsigned cs_mode, int cs_mask_nr);
 };
 
+struct amd64_family_flags {
+	/*
+	 * Indicates that the system supports the new register offsets, etc.
+	 * first introduced with Family 19h Model 10h.
+	 */
+	__u64 zn_regs_v2	: 1,
+
+	      __reserved	: 63;
+};
+
 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 amd64_family_flags flags;
 	struct low_ops ops;
 };
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM
  2022-02-02 14:43 ` [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM Yazen Ghannam
@ 2022-02-03 13:19   ` William Roche
  2022-02-03 14:09     ` Borislav Petkov
  2022-02-23 20:55   ` Borislav Petkov
  1 sibling, 1 reply; 13+ messages in thread
From: William Roche @ 2022-02-03 13:19 UTC (permalink / raw)
  To: Yazen Ghannam, linux-edac
  Cc: linux-kernel, bp, mchehab, tony.luck, james.morse, rric,
	Smita.KoralahalliChannabasappa

As we are moving the dram_type cached date from pvt to umc for family >= 
0x17, should we also add a small comment for the dram_type field in the 
amd64_pvt structure to indicate that ?

Something like that for example:

@@ -385,7 +385,7 @@
      /* place to store error injection parameters prior to issue */
      struct error_injection injection;

-    /* cache the dram_type */
+    /* cache the dram_type for family<0x17 */
      enum mem_type dram_type;

      struct amd64_umc *umc;    /* UMC registers */


Just a suggestion.
The code looks good to me.

Reviewed-by: William Roche <william.roche@oracle.com>

W.

On 02/02/2022 15:43, Yazen Ghannam wrote:
> Current AMD systems allow mixing of DIMM types within a system. However,
> DIMMs within a channel, i.e. managed by a single Unified Memory
> Controller (UMC), must be of the same type.
>
> Handle this possible configuration by checking and setting the memory
> type for each individual "UMC" structure.
>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
> Link:
> https://lore.kernel.org/r/20211228200615.412999-2-yazen.ghannam@amd.com
>
> v3->v4:
> * Cache dram_type in struct umc.
>
> v2->v3:
> * Change patch to properly handle systems with different DIMM types.
>
> v1->v2:
> * Was patch 3 in v1.
> * Update commit message.
>
>   drivers/edac/amd64_edac.c | 47 +++++++++++++++++++++++++++++----------
>   drivers/edac/amd64_edac.h |  3 +++
>   2 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index fba609ada0e6..49e384207ce0 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1429,7 +1429,7 @@ static void __dump_misc_regs_df(struct amd64_pvt *pvt)
>   		edac_dbg(1, "UMC%d x16 DIMMs present: %s\n",
>   				i, (umc->dimm_cfg & BIT(7)) ? "yes" : "no");
>   
> -		if (pvt->dram_type == MEM_LRDDR4) {
> +		if (umc->dram_type == MEM_LRDDR4) {
>   			amd_smn_read(pvt->mc_node_id, umc_base + UMCCH_ADDR_CFG, &tmp);
>   			edac_dbg(1, "UMC%d LRDIMM %dx rank multiply\n",
>   					i, 1 << ((tmp >> 4) & 0x3));
> @@ -1616,19 +1616,40 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
>   	}
>   }
>   
> +static void _determine_memory_type_df(struct amd64_umc *umc)
> +{
> +	if (!(umc->sdp_ctrl & UMC_SDP_INIT)) {
> +		umc->dram_type = MEM_EMPTY;
> +		return;
> +	}
> +
> +	if (umc->dimm_cfg & BIT(5))
> +		umc->dram_type = MEM_LRDDR4;
> +	else if (umc->dimm_cfg & BIT(4))
> +		umc->dram_type = MEM_RDDR4;
> +	else
> +		umc->dram_type = MEM_DDR4;
> +}
> +
> +static void determine_memory_type_df(struct amd64_pvt *pvt)
> +{
> +	struct amd64_umc *umc;
> +	u32 i;
> +
> +	for_each_umc(i) {
> +		umc = &pvt->umc[i];
> +
> +		_determine_memory_type_df(umc);
> +		edac_dbg(1, "  UMC%d DIMM type: %s\n", i, edac_mem_types[umc->dram_type]);
> +	}
> +}
> +
>   static void 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;
> -	}
> +	if (pvt->umc)
> +		return determine_memory_type_df(pvt);
>   
>   	switch (pvt->fam) {
>   	case 0xf:
> @@ -3452,7 +3473,9 @@ static void read_mc_regs(struct amd64_pvt *pvt)
>   	read_dct_base_mask(pvt);
>   
>   	determine_memory_type(pvt);
> -	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
> +
> +	if (!pvt->umc)
> +		edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
>   
>   	determine_ecc_sym_sz(pvt);
>   }
> @@ -3548,7 +3571,7 @@ static int init_csrows_df(struct mem_ctl_info *mci)
>   					pvt->mc_node_id, cs);
>   
>   			dimm->nr_pages = get_csrow_nr_pages(pvt, umc, cs);
> -			dimm->mtype = pvt->dram_type;
> +			dimm->mtype = pvt->umc[umc].dram_type;
>   			dimm->edac_mode = edac_mode;
>   			dimm->dtype = dev_type;
>   			dimm->grain = 64;
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 352bda9803f6..09ad28299c57 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -344,6 +344,9 @@ struct amd64_umc {
>   	u32 sdp_ctrl;		/* SDP Control reg */
>   	u32 ecc_ctrl;		/* DRAM ECC Control reg */
>   	u32 umc_cap_hi;		/* Capabilities High reg */
> +
> +	/* cache the dram_type */
> +	enum mem_type dram_type;
>   };
>   
>   struct amd64_pvt {

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 2/2] EDAC/amd64: Add new register offset support and related changes
  2022-02-02 14:43 ` [PATCH v4 2/2] EDAC/amd64: Add new register offset support and related changes Yazen Ghannam
@ 2022-02-03 13:19   ` William Roche
  0 siblings, 0 replies; 13+ messages in thread
From: William Roche @ 2022-02-03 13:19 UTC (permalink / raw)
  To: Yazen Ghannam, linux-edac
  Cc: linux-kernel, bp, mchehab, tony.luck, james.morse, rric,
	Smita.KoralahalliChannabasappa

This version is clearer according to me, and the dimm value can continue 
to be used in the debug messages at the end of the 
f17_addr_mask_to_cs_size function.
Thank you.

Reviewed-by: William Roche <william.roche@oracle.com>

W.


On 02/02/2022 15:43, Yazen Ghannam wrote:
> Introduce a "family flags" bitmask that can be used to indicate any
> special behavior needed on a per-family basis.
>
> Add a flag to indicate a system uses the new register offsets introduced
> with Family 19h Model 10h.
>
> Use this flag to account for register offset changes, a new bitfield
> indicating DDR5 use on a memory controller, and to set the proper number
> of chip select masks.
>
> Rework f17_addr_mask_to_cs_size() to properly handle the change in chip
> select masks. And update code comments to reflect the updated Chip
> Select, DIMM, and Mask relationships.
>
> [uninitiliazed variable warning]
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
> Link:
> https://lore.kernel.org/r/20211228200615.412999-3-yazen.ghannam@amd.com
>
> v3->v4:
> * Use a single helper function for new register offsets.
> * Fix an uninitialized variable warning.
>
> v2->v3:
> * Adjust variable names to explicitly show what they represent.
> * Update code comment to give more detail on CS/MASK/DIMM layout.
>
> v1->v2:
> * Was patch 4 in v1.
> * Change "has_ddr5" flag to "zn_regs_v2".
> * Drop flag check helper function.
> * Update determine_memory_type() to check bitfield for DDR5.
> * Update code comments.
>
>   drivers/edac/amd64_edac.c | 80 +++++++++++++++++++++++++++++++--------
>   drivers/edac/amd64_edac.h | 14 +++++++
>   2 files changed, 78 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 49e384207ce0..5806fe657373 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -15,6 +15,21 @@ static struct msr __percpu *msrs;
>   
>   static struct amd64_family_type *fam_type;
>   
> +static inline u32 get_umc_reg(u32 reg)
> +{
> +	if (!fam_type->flags.zn_regs_v2)
> +		return reg;
> +
> +	switch (reg) {
> +	case UMCCH_ADDR_CFG:		return UMCCH_ADDR_CFG_DDR5;
> +	case UMCCH_ADDR_MASK_SEC:	return UMCCH_ADDR_MASK_SEC_DDR5;
> +	case UMCCH_DIMM_CFG:		return UMCCH_DIMM_CFG_DDR5;
> +	}
> +
> +	WARN_ONCE(1, "%s: unknown register 0x%x", __func__, reg);
> +	return 0;
> +}
> +
>   /* Per-node stuff */
>   static struct ecc_settings **ecc_stngs;
>   
> @@ -1429,8 +1444,10 @@ static void __dump_misc_regs_df(struct amd64_pvt *pvt)
>   		edac_dbg(1, "UMC%d x16 DIMMs present: %s\n",
>   				i, (umc->dimm_cfg & BIT(7)) ? "yes" : "no");
>   
> -		if (umc->dram_type == MEM_LRDDR4) {
> -			amd_smn_read(pvt->mc_node_id, umc_base + UMCCH_ADDR_CFG, &tmp);
> +		if (umc->dram_type == MEM_LRDDR4 || umc->dram_type == MEM_LRDDR5) {
> +			amd_smn_read(pvt->mc_node_id,
> +				     umc_base + get_umc_reg(UMCCH_ADDR_CFG),
> +				     &tmp);
>   			edac_dbg(1, "UMC%d LRDIMM %dx rank multiply\n",
>   					i, 1 << ((tmp >> 4) & 0x3));
>   		}
> @@ -1505,7 +1522,7 @@ static void prep_chip_selects(struct amd64_pvt *pvt)
>   
>   		for_each_umc(umc) {
>   			pvt->csels[umc].b_cnt = 4;
> -			pvt->csels[umc].m_cnt = 2;
> +			pvt->csels[umc].m_cnt = fam_type->flags.zn_regs_v2 ? 4 : 2;
>   		}
>   
>   	} else {
> @@ -1545,7 +1562,7 @@ static void read_umc_base_mask(struct amd64_pvt *pvt)
>   		}
>   
>   		umc_mask_reg = get_umc_base(umc) + UMCCH_ADDR_MASK;
> -		umc_mask_reg_sec = get_umc_base(umc) + UMCCH_ADDR_MASK_SEC;
> +		umc_mask_reg_sec = get_umc_base(umc) + get_umc_reg(UMCCH_ADDR_MASK_SEC);
>   
>   		for_each_chip_select_mask(cs, umc, pvt) {
>   			mask = &pvt->csels[umc].csmasks[cs];
> @@ -1623,12 +1640,25 @@ static void _determine_memory_type_df(struct amd64_umc *umc)
>   		return;
>   	}
>   
> -	if (umc->dimm_cfg & BIT(5))
> -		umc->dram_type = MEM_LRDDR4;
> -	else if (umc->dimm_cfg & BIT(4))
> -		umc->dram_type = MEM_RDDR4;
> -	else
> -		umc->dram_type = MEM_DDR4;
> +	/*
> +	 * Check if the system supports the "DDR Type" field in UMC Config
> +	 * and has DDR5 DIMMs in use.
> +	 */
> +	if (fam_type->flags.zn_regs_v2 && ((umc->umc_cfg & GENMASK(2, 0)) == 0x1)) {
> +		if (umc->dimm_cfg & BIT(5))
> +			umc->dram_type = MEM_LRDDR5;
> +		else if (umc->dimm_cfg & BIT(4))
> +			umc->dram_type = MEM_RDDR5;
> +		else
> +			umc->dram_type = MEM_DDR5;
> +	} else {
> +		if (umc->dimm_cfg & BIT(5))
> +			umc->dram_type = MEM_LRDDR4;
> +		else if (umc->dimm_cfg & BIT(4))
> +			umc->dram_type = MEM_RDDR4;
> +		else
> +			umc->dram_type = MEM_DDR4;
> +	}
>   }
>   
>   static void determine_memory_type_df(struct amd64_pvt *pvt)
> @@ -2170,6 +2200,7 @@ static int f17_addr_mask_to_cs_size(struct amd64_pvt *pvt, u8 umc,
>   {
>   	u32 addr_mask_orig, addr_mask_deinterleaved;
>   	u32 msb, weight, num_zero_bits;
> +	int cs_mask_nr = csrow_nr;
>   	int dimm, size = 0;
>   
>   	/* No Chip Selects are enabled. */
> @@ -2185,17 +2216,33 @@ static int f17_addr_mask_to_cs_size(struct amd64_pvt *pvt, u8 umc,
>   		return size;
>   
>   	/*
> -	 * There is one mask per DIMM, and two Chip Selects per DIMM.
> -	 *	CS0 and CS1 -> DIMM0
> -	 *	CS2 and CS3 -> DIMM1
> +	 * Family 17h introduced systems with one mask per DIMM,
> +	 * and two Chip Selects per DIMM.
> +	 *
> +	 *	CS0 and CS1 -> MASK0 / DIMM0
> +	 *	CS2 and CS3 -> MASK1 / DIMM1
> +	 *
> +	 * Family 19h Model 10h introduced systems with one mask per Chip Select,
> +	 * and two Chip Selects per DIMM.
> +	 *
> +	 *	CS0 -> MASK0 -> DIMM0
> +	 *	CS1 -> MASK1 -> DIMM0
> +	 *	CS2 -> MASK2 -> DIMM1
> +	 *	CS3 -> MASK3 -> DIMM1
> +	 *
> +	 * Keep the mask number equal to the Chip Select number for newer systems,
> +	 * and shift the mask number for older systems.
>   	 */
>   	dimm = csrow_nr >> 1;
>   
> +	if (!fam_type->flags.zn_regs_v2)
> +		cs_mask_nr >>= 1;
> +
>   	/* Asymmetric dual-rank DIMM support. */
>   	if ((csrow_nr & 1) && (cs_mode & CS_ODD_SECONDARY))
> -		addr_mask_orig = pvt->csels[umc].csmasks_sec[dimm];
> +		addr_mask_orig = pvt->csels[umc].csmasks_sec[cs_mask_nr];
>   	else
> -		addr_mask_orig = pvt->csels[umc].csmasks[dimm];
> +		addr_mask_orig = pvt->csels[umc].csmasks[cs_mask_nr];
>   
>   	/*
>   	 * The number of zero bits in the mask is equal to the number of bits
> @@ -2951,6 +2998,7 @@ static struct amd64_family_type family_types[] = {
>   		.f0_id = PCI_DEVICE_ID_AMD_19H_M10H_DF_F0,
>   		.f6_id = PCI_DEVICE_ID_AMD_19H_M10H_DF_F6,
>   		.max_mcs = 12,
> +		.flags.zn_regs_v2 = 1,
>   		.ops = {
>   			.early_channel_count	= f17_early_channel_count,
>   			.dbam_to_cs		= f17_addr_mask_to_cs_size,
> @@ -3389,7 +3437,7 @@ static void __read_mc_regs_df(struct amd64_pvt *pvt)
>   		umc_base = get_umc_base(i);
>   		umc = &pvt->umc[i];
>   
> -		amd_smn_read(nid, umc_base + UMCCH_DIMM_CFG, &umc->dimm_cfg);
> +		amd_smn_read(nid, umc_base + get_umc_reg(UMCCH_DIMM_CFG), &umc->dimm_cfg);
>   		amd_smn_read(nid, umc_base + UMCCH_UMC_CFG, &umc->umc_cfg);
>   		amd_smn_read(nid, umc_base + UMCCH_SDP_CTRL, &umc->sdp_ctrl);
>   		amd_smn_read(nid, umc_base + UMCCH_ECC_CTRL, &umc->ecc_ctrl);
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 09ad28299c57..6f8147abfa71 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -273,8 +273,11 @@
>   #define UMCCH_BASE_ADDR_SEC		0x10
>   #define UMCCH_ADDR_MASK			0x20
>   #define UMCCH_ADDR_MASK_SEC		0x28
> +#define UMCCH_ADDR_MASK_SEC_DDR5	0x30
>   #define UMCCH_ADDR_CFG			0x30
> +#define UMCCH_ADDR_CFG_DDR5		0x40
>   #define UMCCH_DIMM_CFG			0x80
> +#define UMCCH_DIMM_CFG_DDR5		0x90
>   #define UMCCH_UMC_CFG			0x100
>   #define UMCCH_SDP_CTRL			0x104
>   #define UMCCH_ECC_CTRL			0x14C
> @@ -483,11 +486,22 @@ struct low_ops {
>   					 unsigned cs_mode, int cs_mask_nr);
>   };
>   
> +struct amd64_family_flags {
> +	/*
> +	 * Indicates that the system supports the new register offsets, etc.
> +	 * first introduced with Family 19h Model 10h.
> +	 */
> +	__u64 zn_regs_v2	: 1,
> +
> +	      __reserved	: 63;
> +};
> +
>   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 amd64_family_flags flags;
>   	struct low_ops ops;
>   };
>   

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM
  2022-02-03 13:19   ` William Roche
@ 2022-02-03 14:09     ` Borislav Petkov
  2022-02-03 15:46       ` William Roche
  0 siblings, 1 reply; 13+ messages in thread
From: Borislav Petkov @ 2022-02-03 14:09 UTC (permalink / raw)
  To: William Roche
  Cc: Yazen Ghannam, linux-edac, linux-kernel, mchehab, tony.luck,
	james.morse, rric, Smita.KoralahalliChannabasappa

On Thu, Feb 03, 2022 at 02:19:19PM +0100, William Roche wrote:
> As we are moving the dram_type cached date from pvt to umc for family >=
> 0x17, should we also add a small comment for the dram_type field in the
> amd64_pvt structure to indicate that ?

Who would be that comment for? People who are looking at the code, so
that they know which is which?

> Something like that for example:
> 
> @@ -385,7 +385,7 @@
>      /* place to store error injection parameters prior to issue */
>      struct error_injection injection;
> 
> -    /* cache the dram_type */
> +    /* cache the dram_type for family<0x17 */
>      enum mem_type dram_type;
> 
>      struct amd64_umc *umc;    /* UMC registers */
> 
> 
> Just a suggestion.
> The code looks good to me.
> 
> Reviewed-by: William Roche <william.roche@oracle.com>
> 
> W.

Btw, I'd appreciate it if you do not top-post.

Thx.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM
  2022-02-03 14:09     ` Borislav Petkov
@ 2022-02-03 15:46       ` William Roche
  2022-02-04 15:51         ` Yazen Ghannam
  0 siblings, 1 reply; 13+ messages in thread
From: William Roche @ 2022-02-03 15:46 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Yazen Ghannam, linux-edac, linux-kernel, mchehab, tony.luck,
	james.morse, rric, Smita.KoralahalliChannabasappa

On 03/02/2022 15:09, Borislav Petkov wrote:

> On Thu, Feb 03, 2022 at 02:19:19PM +0100, William Roche wrote:
>> As we are moving the dram_type cached date from pvt to umc for family >=
>> 0x17, should we also add a small comment for the dram_type field in the
>> amd64_pvt structure to indicate that ?
> Who would be that comment for? People who are looking at the code, so
> that they know which is which?

Yes, it could be a hint about the use case of this field.
Of course we could be more complete and also comment the umc field use 
in this same structure that depends on the family higher or lower than 
17 too.
But I had the impression that the creation of a new dram_type cache 
field would be clarified by a comment on the old location, that's it.
It's up to Yazen and you to include or not  this small addition about 
dram_type.

>
>> Something like that for example:
>>
>> @@ -385,7 +385,7 @@
>>       /* place to store error injection parameters prior to issue */
>>       struct error_injection injection;
>>
>> -    /* cache the dram_type */
>> +    /* cache the dram_type for family<0x17 */
>>       enum mem_type dram_type;
>>
>>       struct amd64_umc *umc;    /* UMC registers */
>>
>>
>> Just a suggestion.
>> The code looks good to me.
>>
>> Reviewed-by: William Roche <william.roche@oracle.com>
>>
>> W.
> Btw, I'd appreciate it if you do not top-post.
>
> Thx.

Sure, sorry.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM
  2022-02-03 15:46       ` William Roche
@ 2022-02-04 15:51         ` Yazen Ghannam
  2022-02-04 17:20           ` William Roche
  0 siblings, 1 reply; 13+ messages in thread
From: Yazen Ghannam @ 2022-02-04 15:51 UTC (permalink / raw)
  To: William Roche
  Cc: Borislav Petkov, linux-edac, linux-kernel, mchehab, tony.luck,
	james.morse, rric, Smita.KoralahalliChannabasappa

On Thu, Feb 03, 2022 at 04:46:44PM +0100, William Roche wrote:
> On 03/02/2022 15:09, Borislav Petkov wrote:
> 
> > On Thu, Feb 03, 2022 at 02:19:19PM +0100, William Roche wrote:
> > > As we are moving the dram_type cached date from pvt to umc for family >=
> > > 0x17, should we also add a small comment for the dram_type field in the
> > > amd64_pvt structure to indicate that ?
> > Who would be that comment for? People who are looking at the code, so
> > that they know which is which?
> 
> Yes, it could be a hint about the use case of this field.
> Of course we could be more complete and also comment the umc field use in
> this same structure that depends on the family higher or lower than 17 too.
> But I had the impression that the creation of a new dram_type cache field
> would be clarified by a comment on the old location, that's it.
> It's up to Yazen and you to include or not  this small addition about
> dram_type.
>

Thanks William for the review.

I think this is a good suggestion. I think it could be a bit more verbose.
Please see below.

Thanks,
Yazen

---
 
From 028207619fb01008a2defc65183cbd30f98c021f Mon Sep 17 00:00:00 2001
From: Yazen Ghannam <yazen.ghannam@amd.com>
Date: Fri, 4 Feb 2022 15:10:52 +0000
Subject: [PATCH] EDAC/amd64: Comment on which dram_type to use

A copy of enum mem_type was added to struct amd64_umc so that memory
type can be properly identified on each independent Unified Memory
Controller.

Add a comment to the original struct amd64_pvt variable to indicate it
shouldn't be used on newer systems.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/edac/amd64_edac.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 6f8147abfa71..38e5ad95d010 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -397,7 +397,12 @@ struct amd64_pvt {
 	/* place to store error injection parameters prior to issue */
 	struct error_injection injection;
 
-	/* cache the dram_type */
+	/*
+	 * cache the dram_type
+	 *
+	 * NOTE: Don't use this for Family 17h and later.
+	 *	 Use dram_type in struct amd64_umc instead.
+	 */
 	enum mem_type dram_type;
 
 	struct amd64_umc *umc;	/* UMC registers */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM
  2022-02-04 15:51         ` Yazen Ghannam
@ 2022-02-04 17:20           ` William Roche
  0 siblings, 0 replies; 13+ messages in thread
From: William Roche @ 2022-02-04 17:20 UTC (permalink / raw)
  To: Yazen Ghannam
  Cc: Borislav Petkov, linux-edac, linux-kernel, mchehab, tony.luck,
	james.morse, rric, Smita.KoralahalliChannabasappa

On 04/02/2022 16:51, Yazen Ghannam wrote:

>
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 6f8147abfa71..38e5ad95d010 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -397,7 +397,12 @@ struct amd64_pvt {
>   	/* place to store error injection parameters prior to issue */
>   	struct error_injection injection;
>   
> -	/* cache the dram_type */
> +	/*
> +	 * cache the dram_type
> +	 *
> +	 * NOTE: Don't use this for Family 17h and later.
> +	 *	 Use dram_type in struct amd64_umc instead.
> +	 */
>   	enum mem_type dram_type;
>   
>   	struct amd64_umc *umc;	/* UMC registers */

It works for me Yazen.
Thanks!


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM
  2022-02-02 14:43 ` [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM Yazen Ghannam
  2022-02-03 13:19   ` William Roche
@ 2022-02-23 20:55   ` Borislav Petkov
  2022-02-24 18:18     ` Yazen Ghannam
  1 sibling, 1 reply; 13+ messages in thread
From: Borislav Petkov @ 2022-02-23 20:55 UTC (permalink / raw)
  To: Yazen Ghannam
  Cc: linux-edac, linux-kernel, mchehab, tony.luck, james.morse, rric,
	Smita.KoralahalliChannabasappa, william.roche

On Wed, Feb 02, 2022 at 02:43:06PM +0000, Yazen Ghannam wrote:
> +static void _determine_memory_type_df(struct amd64_umc *umc)

You don't need this function, right?

IOW, here's what I've applied:

---
From: Yazen Ghannam <yazen.ghannam@amd.com>
Date: Wed, 2 Feb 2022 14:43:06 +0000
Subject: [PATCH] EDAC/amd64: Set memory type per DIMM

Current AMD systems allow mixing of DIMM types within a system. However,
DIMMs within a channel, i.e. managed by a single Unified Memory
Controller (UMC), must be of the same type.

Handle this possible configuration by checking and setting the memory
type for each individual "UMC" structure.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: William Roche <william.roche@oracle.com>
Link: https://lore.kernel.org/r/20220202144307.2678405-2-yazen.ghannam@amd.com
---
 drivers/edac/amd64_edac.c | 43 ++++++++++++++++++++++++++++-----------
 drivers/edac/amd64_edac.h | 10 ++++++++-
 2 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index fba609ada0e6..388b072daa94 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1429,7 +1429,7 @@ static void __dump_misc_regs_df(struct amd64_pvt *pvt)
 		edac_dbg(1, "UMC%d x16 DIMMs present: %s\n",
 				i, (umc->dimm_cfg & BIT(7)) ? "yes" : "no");
 
-		if (pvt->dram_type == MEM_LRDDR4) {
+		if (umc->dram_type == MEM_LRDDR4) {
 			amd_smn_read(pvt->mc_node_id, umc_base + UMCCH_ADDR_CFG, &tmp);
 			edac_dbg(1, "UMC%d LRDIMM %dx rank multiply\n",
 					i, 1 << ((tmp >> 4) & 0x3));
@@ -1616,19 +1616,36 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
 	}
 }
 
-static void determine_memory_type(struct amd64_pvt *pvt)
+static void determine_memory_type_df(struct amd64_pvt *pvt)
 {
-	u32 dram_ctrl, dcsm;
+	struct amd64_umc *umc;
+	u32 i;
 
-	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;
+	for_each_umc(i) {
+		umc = &pvt->umc[i];
+
+		if (!(umc->sdp_ctrl & UMC_SDP_INIT)) {
+			umc->dram_type = MEM_EMPTY;
+			continue;
+		}
+
+		if (umc->dimm_cfg & BIT(5))
+			umc->dram_type = MEM_LRDDR4;
+		else if (umc->dimm_cfg & BIT(4))
+			umc->dram_type = MEM_RDDR4;
 		else
-			pvt->dram_type = MEM_DDR4;
-		return;
+			umc->dram_type = MEM_DDR4;
+
+		edac_dbg(1, "  UMC%d DIMM type: %s\n", i, edac_mem_types[umc->dram_type]);
 	}
+}
+
+static void determine_memory_type(struct amd64_pvt *pvt)
+{
+	u32 dram_ctrl, dcsm;
+
+	if (pvt->umc)
+		return determine_memory_type_df(pvt);
 
 	switch (pvt->fam) {
 	case 0xf:
@@ -3452,7 +3469,9 @@ static void read_mc_regs(struct amd64_pvt *pvt)
 	read_dct_base_mask(pvt);
 
 	determine_memory_type(pvt);
-	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
+
+	if (!pvt->umc)
+		edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
 
 	determine_ecc_sym_sz(pvt);
 }
@@ -3548,7 +3567,7 @@ static int init_csrows_df(struct mem_ctl_info *mci)
 					pvt->mc_node_id, cs);
 
 			dimm->nr_pages = get_csrow_nr_pages(pvt, umc, cs);
-			dimm->mtype = pvt->dram_type;
+			dimm->mtype = pvt->umc[umc].dram_type;
 			dimm->edac_mode = edac_mode;
 			dimm->dtype = dev_type;
 			dimm->grain = 64;
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 352bda9803f6..6b8742369f9d 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -344,6 +344,9 @@ struct amd64_umc {
 	u32 sdp_ctrl;		/* SDP Control reg */
 	u32 ecc_ctrl;		/* DRAM ECC Control reg */
 	u32 umc_cap_hi;		/* Capabilities High reg */
+
+	/* cache the dram_type */
+	enum mem_type dram_type;
 };
 
 struct amd64_pvt {
@@ -391,7 +394,12 @@ struct amd64_pvt {
 	/* place to store error injection parameters prior to issue */
 	struct error_injection injection;
 
-	/* cache the dram_type */
+	/*
+	 * cache the dram_type
+	 *
+	 * NOTE: Don't use this for Family 17h and later.
+	 *	 Use dram_type in struct amd64_umc instead.
+	 */
 	enum mem_type dram_type;
 
 	struct amd64_umc *umc;	/* UMC registers */
-- 
2.29.2

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM
  2022-02-23 20:55   ` Borislav Petkov
@ 2022-02-24 18:18     ` Yazen Ghannam
  0 siblings, 0 replies; 13+ messages in thread
From: Yazen Ghannam @ 2022-02-24 18:18 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-edac, linux-kernel, mchehab, tony.luck, james.morse, rric,
	Smita.KoralahalliChannabasappa, william.roche

On Wed, Feb 23, 2022 at 09:55:08PM +0100, Borislav Petkov wrote:
> On Wed, Feb 02, 2022 at 02:43:06PM +0000, Yazen Ghannam wrote:
> > +static void _determine_memory_type_df(struct amd64_umc *umc)
> 
> You don't need this function, right?
> 
> IOW, here's what I've applied:
> 

...

>  
> -static void determine_memory_type(struct amd64_pvt *pvt)
> +static void determine_memory_type_df(struct amd64_pvt *pvt)
>  {
> -	u32 dram_ctrl, dcsm;
> +	struct amd64_umc *umc;
> +	u32 i;
>  
> -	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;
> +	for_each_umc(i) {
> +		umc = &pvt->umc[i];
> +
> +		if (!(umc->sdp_ctrl & UMC_SDP_INIT)) {
> +			umc->dram_type = MEM_EMPTY;
> +			continue;
> +		}
> +
> +		if (umc->dimm_cfg & BIT(5))
> +			umc->dram_type = MEM_LRDDR4;
> +		else if (umc->dimm_cfg & BIT(4))
> +			umc->dram_type = MEM_RDDR4;
>  		else
> -			pvt->dram_type = MEM_DDR4;
> -		return;
> +			umc->dram_type = MEM_DDR4;
> +
> +		edac_dbg(1, "  UMC%d DIMM type: %s\n", i, edac_mem_types[umc->dram_type]);
>  	}
> +}

Ah, I see. You got rid of the extra helper function. Makes sense and looks
okay to me.

Thanks,
Yazen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates
  2022-02-02 14:43 [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates Yazen Ghannam
  2022-02-02 14:43 ` [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM Yazen Ghannam
  2022-02-02 14:43 ` [PATCH v4 2/2] EDAC/amd64: Add new register offset support and related changes Yazen Ghannam
@ 2022-03-06 16:18 ` Borislav Petkov
  2022-03-09 21:05   ` Yazen Ghannam
  2 siblings, 1 reply; 13+ messages in thread
From: Borislav Petkov @ 2022-03-06 16:18 UTC (permalink / raw)
  To: Yazen Ghannam
  Cc: linux-edac, linux-kernel, mchehab, tony.luck, james.morse, rric,
	Smita.KoralahalliChannabasappa, william.roche

On Wed, Feb 02, 2022 at 02:43:05PM +0000, Yazen Ghannam wrote:
> Hi all,
> 
> This set adds support for AMD Family 19h Models 10h-1Fh and A0h-AFh.
> 
> Patch 1 sets the "memory type" per DIMM. 
> 
> Patch 2 adds register offset and other minor changes introduced with
> these new models.
> 
> Thanks,
> Yazen
> 
> Link:
> https://lore.kernel.org/r/20211228200615.412999-1-yazen.ghannam@amd.com
> 
> v3->v4:
> * Updated patch 1 to cache dram_type in struct umc.
> * Fixed uninitiliazed variable warning in patch 2.
> * Switched to a single register helper function.
> 
> v2->v3:
> * Patch 1 completely reworked.
> * Patch 2 updated based on comments from William.
> 
> Yazen Ghannam (2):
>   EDAC/amd64: Set memory type per DIMM
>   EDAC/amd64: Add new register offset support and related changes
> 
>  drivers/edac/amd64_edac.c | 113 +++++++++++++++++++++++++++++++-------
>  drivers/edac/amd64_edac.h |  17 ++++++
>  2 files changed, 109 insertions(+), 21 deletions(-)

Queued, thanks.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates
  2022-03-06 16:18 ` [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates Borislav Petkov
@ 2022-03-09 21:05   ` Yazen Ghannam
  0 siblings, 0 replies; 13+ messages in thread
From: Yazen Ghannam @ 2022-03-09 21:05 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-edac, linux-kernel, mchehab, tony.luck, james.morse, rric,
	Smita.KoralahalliChannabasappa, william.roche

On Sun, Mar 06, 2022 at 05:18:31PM +0100, Borislav Petkov wrote:
> 
> Queued, thanks.
>

Thank you!

-Yazen 

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-03-09 21:05 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-02 14:43 [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates Yazen Ghannam
2022-02-02 14:43 ` [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM Yazen Ghannam
2022-02-03 13:19   ` William Roche
2022-02-03 14:09     ` Borislav Petkov
2022-02-03 15:46       ` William Roche
2022-02-04 15:51         ` Yazen Ghannam
2022-02-04 17:20           ` William Roche
2022-02-23 20:55   ` Borislav Petkov
2022-02-24 18:18     ` Yazen Ghannam
2022-02-02 14:43 ` [PATCH v4 2/2] EDAC/amd64: Add new register offset support and related changes Yazen Ghannam
2022-02-03 13:19   ` William Roche
2022-03-06 16:18 ` [PATCH v4 0/2] AMD Family 19h Models 10h-1Fh Updates Borislav Petkov
2022-03-09 21:05   ` Yazen Ghannam

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