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

[Borislav Petkov <bp@alien8.de>]

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?

> @@ -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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette