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

Chatradhi, Naveen Krishna
2022-03-31T12:20:15Z

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