Re: [PATCH v3 2/2] EDAC/amd64: Add new register offset support and related changes

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

On Fri, Jan 21, 2022 at 01:25:21PM +0100, Borislav Petkov wrote:
> On Tue, Dec 28, 2021 at 08:06:15PM +0000, Yazen Ghannam wrote:
> > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> > index 4db92c77276f..a299c361a904 100644
> > --- a/drivers/edac/amd64_edac.c
> > +++ b/drivers/edac/amd64_edac.c
> > @@ -15,6 +15,31 @@ static struct msr __percpu *msrs;
> >  
> >  static struct amd64_family_type *fam_type;
> >  
> > +/* Family flag helpers */
> > +static inline u64 get_addr_cfg(void)
> > +{
> > +	if (fam_type->flags.zn_regs_v2)
> > +		return UMCCH_ADDR_CFG_DDR5;
> > +
> > +	return UMCCH_ADDR_CFG;
> > +}
> > +
> > +static inline u64 get_addr_mask_sec(void)
> > +{
> > +	if (fam_type->flags.zn_regs_v2)
> > +		return UMCCH_ADDR_MASK_SEC_DDR5;
> > +
> > +	return UMCCH_ADDR_MASK_SEC;
> > +}
> > +
> > +static inline u64 get_dimm_cfg(void)
> > +{
> > +	if (fam_type->flags.zn_regs_v2)
> > +		return UMCCH_DIMM_CFG_DDR5;
> > +
> > +	return UMCCH_DIMM_CFG;
> > +}
> 
> Yeah, you can do it either this way and have a lot of small functions
> or you can do what I did with mca_msr_reg() which is a single mapping
> function you then use everywhere.
> 
> Your call.
>

Yes, I'll do this.
 
> > +
> >  /* Per-node stuff */
> >  static struct ecc_settings **ecc_stngs;
> >  
> > @@ -1429,8 +1454,10 @@ static void __dump_misc_regs_df(struct amd64_pvt *pvt)
> >  		edac_dbg(1, "UMC%d x16 DIMMs present: %s\n",
> >  				i, (umc->dimm_cfg & BIT(7)) ? "yes" : "no");
> >  
> > -		if (pvt->dram_type == MEM_LRDDR4) {
> > -			amd_smn_read(pvt->mc_node_id, umc_base + UMCCH_ADDR_CFG, &tmp);
> > +		if (pvt->dram_type == MEM_LRDDR4 || pvt->dram_type == MEM_LRDDR5) {
> 
> This still keeps the ->dram_type per pvt, which is per memory controller
> in amd64_edac nomenclature.
> 
> But AFAIR, we said last time that the DRAM type is per UMC now, as you
> do in the previous patch.
> 
> Which means, you either have to test umc->dimm_cfg to get the DRAM type
> here or push ->dram_type into the umc struct...
>

Yep, you're right. I'll cache the dram_type in the umc struct.

Thanks,
Yazen