Messages in this thread | | | Date | Thu, 11 Nov 2021 21:53:32 +0530 | Subject | Re: [PATCH v6 3/5] EDAC/amd64: Extend family ops functions | From | "Chatradhi, Naveen Krishna" <> |
| |
Hi Boris
On 11/10/2021 11:15 PM, Borislav Petkov wrote: > [CAUTION: External Email] > > On Thu, Oct 28, 2021 at 06:31:04PM +0530, Naveen Krishna Chatradhi wrote: >> @@ -1217,28 +1214,39 @@ static void dump_misc_regs(struct amd64_pvt *pvt) >> /* >> * See BKDG, F2x[1,0][5C:40], F2[1,0][6C:60] >> */ >> -static void prep_chip_selects(struct amd64_pvt *pvt) >> +static void k8_prep_chip_selects(struct amd64_pvt *pvt) >> { >> if (pvt->fam == 0xf && pvt->ext_model < K8_REV_F) { >> pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8; >> pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 8; >> - } else if (pvt->fam == 0x15 && pvt->model == 0x30) { >> - pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4; >> - pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2; >> - } else if (pvt->fam >= 0x17) { >> - int umc; >> - >> - for_each_umc(umc) { >> - pvt->csels[umc].b_cnt = 4; >> - pvt->csels[umc].m_cnt = 2; >> - } >> - >> - } else { >> + } else if (pvt->fam == 0xf && pvt->ext_model >= K8_REV_F) { >> pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8; >> pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4; >> } > Why is this function looking at the family if it is called a k8_... > function which will be set only on K8? Will modify. > >> } >> >> +static void f15m30_prep_chip_selects(struct amd64_pvt *pvt) >> +{ >> + pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4; >> + pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2; >> +} >> + >> +static void default_prep_chip_selects(struct amd64_pvt *pvt) >> +{ >> + pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8; >> + pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4; >> +} >> + >> +static void f17_prep_chip_selects(struct amd64_pvt *pvt) >> +{ >> + int umc; >> + >> + for_each_umc(umc) { >> + pvt->csels[umc].b_cnt = 4; >> + pvt->csels[umc].m_cnt = 2; >> + } >> +} >> + >> static void read_umc_base_mask(struct amd64_pvt *pvt) >> { >> u32 umc_base_reg, umc_base_reg_sec; >> @@ -1297,11 +1305,6 @@ static void read_dct_base_mask(struct amd64_pvt *pvt) >> { >> int cs; >> >> - prep_chip_selects(pvt); >> - >> - if (pvt->umc) >> - return read_umc_base_mask(pvt); >> - >> for_each_chip_select(cs, 0, pvt) { >> int reg0 = DCSB0 + (cs * 4); >> int reg1 = DCSB1 + (cs * 4); >> @@ -2512,143 +2515,181 @@ static void debug_display_dimm_sizes(struct amd64_pvt *pvt, u8 ctrl) >> } >> } >> >> +/* Prototypes for family specific ops routines */ >> +static int init_csrows(struct mem_ctl_info *mci); >> +static int init_csrows_df(struct mem_ctl_info *mci); >> +static void read_mc_regs(struct amd64_pvt *pvt); >> +static void __read_mc_regs_df(struct amd64_pvt *pvt); >> +static void update_umc_err_info(struct mce *m, struct err_info *err); > Prototypes belong in headers. Sure, this wont be necessary based on your other comments. > >> + >> +static const struct low_ops k8_ops = { > So if you're going to do this, you can go a step further and get rid of > all those static definitions which are completely unused except those of > the current family we're loaded on. > > IOW, you should make all family-specific assignments dynamic and get rid > of family_types and those ops. Here's an example I did for K8: > > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c > index 1029fe84ba2e..5f1686f22947 100644 > --- a/drivers/edac/amd64_edac.c > +++ b/drivers/edac/amd64_edac.c > @@ -3656,8 +3656,18 @@ static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt) > > switch (pvt->fam) { > case 0xf: > - fam_type = &family_types[K8_CPUS]; > - pvt->ops = &family_types[K8_CPUS].ops; > + fam_type->ctl_name = "K8"; > + fam_type->f1_id = PCI_DEVICE_ID_AMD_K8_NB_ADDRMAP; > + fam_type->f2_id = PCI_DEVICE_ID_AMD_K8_NB_MEMCTL; > + fam_type->max_mcs = 2; > + pvt->ops->early_channel_count = k8_early_channel_count; > + pvt->ops->map_sysaddr_to_csrow = k8_map_sysaddr_to_csrow; > + pvt->ops->dbam_to_cs = k8_dbam_to_chip_select; > + pvt->ops->prep_chip_select = k8_prep_chip_selects; > + pvt->ops->get_base_mask = read_dct_base_mask; > + pvt->ops->get_misc_regs = __dump_misc_regs; > + pvt->ops->get_mc_regs = read_mc_regs; > + pvt->ops->populate_csrows = init_csrows; > break; > > case 0x10: > > After that, pvt and fam_type have been properly set up.
Agreed, will create family_XXh_init() under per_family_init()'s switch.
At present, we are handling the family_type in 4/5 of this set. Will club them.
> >> @@ -3735,7 +3784,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_select(pvt); >> + >> + pvt->ops->get_base_mask(pvt); >> + >> + determine_memory_type(pvt); >> + edac_dbg(1, " DIMM type: %s\n", edac_mem_types[pvt->dram_type]); > Move that dbg call at the end of determine_memory_type(). Sure > >> + >> + determine_ecc_sym_sz(pvt); >> >> return 0; >> } >> @@ -3786,7 +3844,7 @@ static int init_one_instance(struct amd64_pvt *pvt) >> >> setup_mci_misc_attrs(mci); >> >> - if (init_csrows(mci)) >> + if (pvt->ops->populate_csrows(mci)) >> mci->edac_cap = EDAC_FLAG_NONE; >> >> ret = -ENODEV; >> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h >> index 85aa820bc165..881ff6322bc9 100644 >> --- a/drivers/edac/amd64_edac.h >> +++ b/drivers/edac/amd64_edac.h >> @@ -467,11 +467,17 @@ struct ecc_settings { >> * functions and per device encoding/decoding logic. >> */ >> struct low_ops { >> - int (*early_channel_count) (struct amd64_pvt *pvt); >> + int (*early_channel_count) (struct amd64_pvt *pvt); >> void (*map_sysaddr_to_csrow) (struct mem_ctl_info *mci, u64 sys_addr, >> struct err_info *); >> - int (*dbam_to_cs) (struct amd64_pvt *pvt, u8 dct, >> + int (*dbam_to_cs) (struct amd64_pvt *pvt, u8 dct, >> unsigned cs_mode, int cs_mask_nr); >> + void (*prep_chip_select) (struct amd64_pvt *pvt); > That name should be "prep_chip_selects" - plural. Got it. > >> + void (*get_base_mask) (struct amd64_pvt *pvt); >> + void (*get_misc_regs) (struct amd64_pvt *pvt); >> + void (*get_mc_regs) (struct amd64_pvt *pvt); >> + int (*populate_csrows) (struct mem_ctl_info *mci); >> + void (*get_umc_err_info) (struct mce *m, struct err_info *err); > WARNING: Unnecessary space before function pointer arguments > #652: FILE: drivers/edac/amd64_edac.h:470: > + int (*early_channel_count) (struct amd64_pvt *pvt); > > WARNING: Unnecessary space before function pointer arguments > #656: FILE: drivers/edac/amd64_edac.h:473: > + int (*dbam_to_cs) (struct amd64_pvt *pvt, u8 dct, > > WARNING: Unnecessary space before function pointer arguments > #658: FILE: drivers/edac/amd64_edac.h:475: > + void (*prep_chip_select) (struct amd64_pvt *pvt); > > WARNING: Unnecessary space before function pointer arguments > #659: FILE: drivers/edac/amd64_edac.h:476: > + void (*get_base_mask) (struct amd64_pvt *pvt); > > WARNING: Unnecessary space before function pointer arguments > #660: FILE: drivers/edac/amd64_edac.h:477: > + void (*get_misc_regs) (struct amd64_pvt *pvt); > > WARNING: Unnecessary space before function pointer arguments > #661: FILE: drivers/edac/amd64_edac.h:478: > + void (*get_mc_regs) (struct amd64_pvt *pvt); > > WARNING: Unnecessary space before function pointer arguments > #662: FILE: drivers/edac/amd64_edac.h:479: > + int (*populate_csrows) (struct mem_ctl_info *mci); > > WARNING: Unnecessary space before function pointer arguments > #663: FILE: drivers/edac/amd64_edac.h:480: > + void (*get_umc_err_info) (struct mce *m, struct err_info *err); > > total: 0 errors, 8 warnings, 507 lines checked > > > Please integrate scripts/checkpatch.pl into your patch creation > workflow. Some of the warnings/errors *actually* make sense. I have noticed these warnings. As there were previous definitions, i continued with similar indentation. Will address all of them. > > -- > Regards/Gruss, > Boris. Thanks for the review. > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7Ca162e192bf9a44cf719308d9a471f62b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637721631674685370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=eLKOAi8ljBkPLL04EVk4q1jTDQ1PzaNBv2Wltll%2FU24%3D&reserved=0
| |