Messages in this thread | | | Date | Mon, 13 Dec 2021 17:46:55 +0000 | From | Yazen Ghannam <> | Subject | Re: [PATCH 4/4] EDAC/amd64: Add DDR5 support and related register changes |
| |
On Fri, Dec 10, 2021 at 01:41:26PM +0100, Borislav Petkov wrote: > On Wed, Dec 08, 2021 at 05:43:56PM +0000, Yazen Ghannam wrote: > > Future AMD systems will support DDR5. > > > > Add support for changes in register addresses for these systems. > > > > Introduce a "family flags" bitmask that can be used to indicate any > > special behavior needed on a per-family basis. > > > > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> > > --- > > drivers/edac/amd64_edac.c | 61 +++++++++++++++++++++++++++++++++++---- > > drivers/edac/amd64_edac.h | 11 +++++++ > > 2 files changed, 66 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c > > index 1df763128483..e37a8e0cef7e 100644 > > --- a/drivers/edac/amd64_edac.c > > +++ b/drivers/edac/amd64_edac.c > > @@ -15,6 +15,36 @@ static struct msr __percpu *msrs; > > > > static struct amd64_family_type *fam_type; > > > > +/* Family flag helpers */ > > +static inline bool has_ddr5(void) > > +{ > > + return fam_type->flags.has_ddr5; > > A flag about ddr5 *and* a function of the same name. Kinda too much, > don't ya think? >
Yeah, you're right. I didn't think about that. I think I'll drop this function and just check the flag directly.
> > @@ -1628,6 +1660,17 @@ static void determine_memory_type(struct amd64_pvt *pvt) > > dimm_cfg |= pvt->umc[i].dimm_cfg; > > } > > > > + /* Check if system supports DDR5 and has DDR5 DIMMs in use. */ > > + if (has_ddr5() && (umc_cfg & BIT(0))) { > > + if (dimm_cfg & BIT(5)) > > + pvt->dram_type = MEM_LRDDR5; > > + else if (dimm_cfg & BIT(4)) > > + pvt->dram_type = MEM_RDDR5; > > + else > > + pvt->dram_type = MEM_DDR5; > > + return; > > + } > > + > > if (dimm_cfg & BIT(5)) > > pvt->dram_type = MEM_LRDDR4; > > else if (dimm_cfg & BIT(4)) > > @@ -2174,8 +2217,13 @@ static int f17_addr_mask_to_cs_size(struct amd64_pvt *pvt, u8 umc, > > * There is one mask per DIMM, and two Chip Selects per DIMM. > > * CS0 and CS1 -> DIMM0 > > * CS2 and CS3 -> DIMM1 > > + * > > + * Systems with DDR5 support have one mask per Chip Select. > > */ > > - dimm = csrow_nr >> 1; > > + if (has_ddr5()) > > + dimm = csrow_nr; > > + else > > + dimm = csrow_nr >> 1; > > > > /* Asymmetric dual-rank DIMM support. */ > > if ((csrow_nr & 1) && (cs_mode & CS_ODD_SECONDARY)) > > @@ -2937,6 +2985,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.has_ddr5 = 1, > > So judging by the name, this means that model 0x10 has DDR5. But I think > you wanna say whether it supports DDR5 or not? > > Or does M10 support DDR5 only? > > But it doesn't look like it from the comment above: > > "Check if system supports DDR5 and has DDR5 DIMMs in use." > > So why is this thing set statically only for this model instead of > detecting from the hw whether there are ddr5 or ddr5 DIMMs and what it > supports? > > And then you can use the defines you just added in patch 1. > > I'm confused. >
Yeah, sorry it's not clear. The purpose of the flag is to indicate some minor changes that show up with future systems like register offsets changes, etc. I didn't want to tie the name to a specific model or core name. I went with DDR5 as a new feature that shows up with these changes, but they're not directly tied to DDR5.
But yes, a system may support DDR5 and DDR4. And this can be detected from the hardware.
What do you think about calling the flag "uses_f19h_m10h_offsets" or something like that? I was trying to avoid family/model in the name, but the code already does this all over. And the convention has been to call something by the first family/model where it shows up.
Thanks, Yazen
| |