| Message ID | 20220202144307.2678405-2-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMD Family 19h Models 10h-1Fh Updates | expand |
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 {
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.
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.
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 */
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!
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 */
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
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 {
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(-)