lkml.org 
[lkml]   [2021]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2021-12-13 18:47    [W:0.085 / U:0.604 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site