Re: [PATCH 06/14] EDAC/amd64: Add get_mc_regs() into pvt->ops

["Chatradhi, Naveen Krishna" <nchatrad@amd.com>]

Hi Yazen,

On 3/28/2022 9:38 PM, Yazen Ghannam wrote:
> On Mon, Feb 28, 2022 at 09:43:46PM +0530, Naveen Krishna Chatradhi wrote:
>> From: Muralidhara M K <muralimk@amd.com>
>>
>> Add function pointer for get_mc_regs() in pvt->ops and assign
>> family specific get_mc_regs() definitions appropriately.
>>
> Please include the "why".
Sure
>
>> Signed-off-by: Muralidhara M K <muralimk@amd.com>
>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>> ---
>> This patch is created by splitting the 5/12th patch in series
>> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
>>
>>   drivers/edac/amd64_edac.c | 77 +++++++++++++++++++++------------------
>>   drivers/edac/amd64_edac.h |  1 +
>>   2 files changed, 43 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
>> index 69c33eb17e4f..713ffe763e64 100644
>> --- a/drivers/edac/amd64_edac.c
>> +++ b/drivers/edac/amd64_edac.c
>> @@ -3214,6 +3214,27 @@ static void f17_determine_ecc_sym_sz(struct amd64_pvt *pvt)
>>   	}
>>   }
>>   
>> +static void read_top_mem_registers(struct amd64_pvt *pvt)
>> +{
>> +	u64 msr_val;
>> +
>> +	/*
>> +	 * Retrieve TOP_MEM and TOP_MEM2; no masking off of reserved bits since
>> +	 * those are Read-As-Zero.
>> +	 */
>> +	rdmsrl(MSR_K8_TOP_MEM1, pvt->top_mem);
>> +	edac_dbg(0, "  TOP_MEM:  0x%016llx\n", pvt->top_mem);
>> +
>> +	/* Check first whether TOP_MEM2 is enabled: */
>> +	rdmsrl(MSR_AMD64_SYSCFG, msr_val);
>> +	if (msr_val & BIT(21)) {
>> +		rdmsrl(MSR_K8_TOP_MEM2, pvt->top_mem2);
>> +		edac_dbg(0, "  TOP_MEM2: 0x%016llx\n", pvt->top_mem2);
>> +	} else {
>> +		edac_dbg(0, "  TOP_MEM2 disabled\n");
>> +	}
> These two values are not used by any code within this module. They are only
> used in debug print statements and debug sysfs entries. I think this code
> should just be removed. An expert user who wants to know TOM and TOM2 can use
> another method, like msr-tools, rather than recompile a kernel with
> CONFIG_EDAC_DEBUG, etc.
Make sense, do you think some users have developed scripts to parse the 
EDAC debug logs ?
>
>> +}
>> +
>>   /*
>>    * Retrieve the hardware registers of the memory controller.
>>    */
>> @@ -3235,6 +3256,8 @@ static void __read_mc_regs_df(struct amd64_pvt *pvt)
>>   		amd_smn_read(nid, umc_base + UMCCH_ECC_CTRL, &umc->ecc_ctrl);
>>   		amd_smn_read(nid, umc_base + UMCCH_UMC_CAP_HI, &umc->umc_cap_hi);
>>   	}
>> +
>> +	amd64_read_pci_cfg(pvt->F0, DF_DHAR, &pvt->dhar);
> "dhar" is not used by any code for Zen-based systems. I think this line can be
> dropped. Reading "dhar" should still be preserved for legacy systems.
>
> This is also the only use of PCI F0. So all the F0 IDs can be removed too. I
> have a patch for this as part of some general code clean up. Let's include
> that with this set also. I think removing TOM/TOM2 code can be included too.
Sure, we can work on this.
>
>>   }
>>   
>>   /*
>> @@ -3244,30 +3267,8 @@ static void __read_mc_regs_df(struct amd64_pvt *pvt)
>>   static void read_mc_regs(struct amd64_pvt *pvt)
>>   {
>>   	unsigned int range;
>> -	u64 msr_val;
>>   
>> -	/*
>> -	 * Retrieve TOP_MEM and TOP_MEM2; no masking off of reserved bits since
>> -	 * those are Read-As-Zero.
>> -	 */
>> -	rdmsrl(MSR_K8_TOP_MEM1, pvt->top_mem);
>> -	edac_dbg(0, "  TOP_MEM:  0x%016llx\n", pvt->top_mem);
>> -
>> -	/* Check first whether TOP_MEM2 is enabled: */
>> -	rdmsrl(MSR_AMD64_SYSCFG, msr_val);
>> -	if (msr_val & BIT(21)) {
>> -		rdmsrl(MSR_K8_TOP_MEM2, pvt->top_mem2);
>> -		edac_dbg(0, "  TOP_MEM2: 0x%016llx\n", pvt->top_mem2);
>> -	} else {
>> -		edac_dbg(0, "  TOP_MEM2 disabled\n");
>> -	}
>> -
>> -	if (pvt->umc) {
>> -		__read_mc_regs_df(pvt);
>> -		amd64_read_pci_cfg(pvt->F0, DF_DHAR, &pvt->dhar);
>> -
>> -		goto skip;
>> -	}
>> +	read_top_mem_registers(pvt);
>>   
>>   	amd64_read_pci_cfg(pvt->F3, NBCAP, &pvt->nbcap);
>>   
>> @@ -3308,16 +3309,6 @@ static void read_mc_regs(struct amd64_pvt *pvt)
>>   		amd64_read_dct_pci_cfg(pvt, 1, DCLR0, &pvt->dclr1);
>>   		amd64_read_dct_pci_cfg(pvt, 1, DCHR0, &pvt->dchr1);
>>   	}
>> -
>> -skip:
>> -	pvt->ops->prep_chip_selects(pvt);
>> -
>> -	pvt->ops->get_base_mask(pvt);
>> -
>> -	pvt->ops->determine_memory_type(pvt);
>> -	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
>> -
>> -	pvt->ops->determine_ecc_sym_sz(pvt);
>>   }
>>   
>>   /*
>> @@ -3792,6 +3783,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->prep_chip_selects		= k8_prep_chip_selects;
>>   		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= read_mc_regs;
> The function names should be more consistent: either both get or read.
>
> The read_mc_regs() function is used for systems with DCTs (i.e. legacy). This
> can be included in the name.
sure
>
>>   		break;
>>   
>>   	case 0x10:
>> @@ -3805,6 +3797,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
>>   		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= read_mc_regs;
>>   		break;
>>   
>>   	case 0x15:
>> @@ -3834,6 +3827,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->get_base_mask			= read_dct_base_mask;
>>   		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= read_mc_regs;
>>   		break;
>>   
>>   	case 0x16:
>> @@ -3853,6 +3847,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->prep_chip_selects		= default_prep_chip_selects;
>>   		pvt->ops->determine_memory_type		= f1x_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f1x_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= read_mc_regs;
>>   		break;
>>   
>>   	case 0x17:
>> @@ -3886,6 +3881,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
>>   		pvt->ops->determine_memory_type		= f17_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= __read_mc_regs_df;
> The underscore prefix can be removed, since this is no longer a helper
> function. Also, the "df" suffix can be removed when changing the name.
>
> Maybe something like this:
>
> pvt->ops->read_mc_regs()    <--- This reads memory controller registers.
>
> read_dct_regs()    <--- Used for DRAM Controllers (DCTs).
>
> read_umc_regs()    <--- Used for Unified Memory Controllers (UMCs).
sure
>
>>   
>>   		if (pvt->fam == 0x18) {
>>   			pvt->ctl_name			= "F18h";
>> @@ -3925,6 +3921,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   		pvt->ops->prep_chip_selects		= f17_prep_chip_selects;
>>   		pvt->ops->determine_memory_type		= f17_determine_memory_type;
>>   		pvt->ops->determine_ecc_sym_sz		= f17_determine_ecc_sym_sz;
>> +		pvt->ops->get_mc_regs			= __read_mc_regs_df;
>>   		break;
>>   
>>   	default:
>> @@ -3935,7 +3932,8 @@ static int per_family_init(struct amd64_pvt *pvt)
>>   	/* ops required for all the families */
>>   	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
>>   	    !pvt->ops->get_base_mask || !pvt->ops->prep_chip_selects ||
>> -	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz) {
>> +	    !pvt->ops->determine_memory_type || !pvt->ops->determine_ecc_sym_sz ||
>> +	    !pvt->ops->get_mc_regs) {
>>   		edac_dbg(1, "Common helper routines not defined.\n");
>>   		return -EFAULT;
>>   	}
>> @@ -3972,7 +3970,16 @@ static int hw_info_get(struct amd64_pvt *pvt)
>>   	if (ret)
>>   		return ret;
>>   
>> -	read_mc_regs(pvt);
>> +	pvt->ops->get_mc_regs(pvt);
>> +
>> +	pvt->ops->prep_chip_selects(pvt);
>> +
>> +	pvt->ops->get_base_mask(pvt);
>> +
>> +	pvt->ops->determine_memory_type(pvt);
>> +	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
> This line should be included in determine_memory_type(). It should be called
> for each PVT on legacy systems and for each UMC on current systems.

sure

Regards,

Naveenk

>
> Thanks,
> Yazen