lkml.org 
[lkml]   [2022]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v4 1/2] EDAC/amd64: Set memory type per DIMM
From
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 {

\
 
 \ /
  Last update: 2022-02-03 14:20    [W:0.176 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site