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

Yazen Ghannam
2022-03-28T16:08:31Z

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

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

> +}
> +
> /*
> * 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.

> }
>
> /*
> @@ -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.

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

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

Thanks,
Yazen