linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Yazen Ghannam <yazen.ghannam@amd.com>
To: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
	mchehab@kernel.org, tony.luck@intel.com, james.morse@arm.com,
	rric@kernel.org, Smita.KoralahalliChannabasappa@amd.com,
	william.roche@oracle.com
Subject: Re: [PATCH 4/4] EDAC/amd64: Add DDR5 support and related register changes
Date: Mon, 13 Dec 2021 17:46:55 +0000	[thread overview]
Message-ID: <YbeHD5PW0sv4O13r@yaz-ubuntu> (raw)
In-Reply-To: <YbNK9jV06al93XDN@zn.tnic>

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

  reply	other threads:[~2021-12-13 17:47 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-08 17:43 [PATCH 0/4] AMD Family 19h Models 10h-1Fh Updates Yazen Ghannam
2021-12-08 17:43 ` [PATCH 1/4] EDAC: Add RDDR5 and LRDDR5 memory types Yazen Ghannam
2021-12-08 17:43 ` [PATCH 2/4] EDAC/amd64: Add support for AMD Family 19h Models 10h-1Fh and A0h-AFh Yazen Ghannam
2021-12-08 17:43 ` [PATCH 3/4] EDAC/amd64: Check register values from all UMCs Yazen Ghannam
2021-12-10 12:34   ` Borislav Petkov
2021-12-13 17:24     ` Yazen Ghannam
2021-12-08 17:43 ` [PATCH 4/4] EDAC/amd64: Add DDR5 support and related register changes Yazen Ghannam
2021-12-10 12:41   ` Borislav Petkov
2021-12-13 17:46     ` Yazen Ghannam [this message]
2021-12-14 16:22       ` Borislav Petkov
2021-12-10 12:44 ` [PATCH 0/4] AMD Family 19h Models 10h-1Fh Updates Borislav Petkov
2021-12-13 17:23   ` Yazen Ghannam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YbeHD5PW0sv4O13r@yaz-ubuntu \
    --to=yazen.ghannam@amd.com \
    --cc=Smita.KoralahalliChannabasappa@amd.com \
    --cc=bp@alien8.de \
    --cc=james.morse@arm.com \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=rric@kernel.org \
    --cc=tony.luck@intel.com \
    --cc=william.roche@oracle.com \
    --subject='Re: [PATCH 4/4] EDAC/amd64: Add DDR5 support and related register changes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).