Re: [PATCH 4/4] EDAC/amd64: Add DDR5 support and related register changes

[Yazen Ghannam <yazen.ghannam@amd.com>]

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