| Message ID | 20220509145534.44912-7-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMD64 EDAC Cleanup and Refactor | expand |
On Mon, May 09, 2022 at 02:55:22PM +0000, Yazen Ghannam wrote: > From: Muralidhara M K <muralidhara.mk@amd.com> > > GPU Nodes will need to set the number of available Chip Selects, i.e. > Base and Mask values, differently than existing systems. A function > pointer should be used rather than introduce another branching condition. Yeah, it looks to me like all that detection logic should be split eventually. Looking at read_mc_regs(), it has if (pvt->umc) { __read_mc_regs_df(pvt); goto skip; } at the top, then a whole bunch of legacy stuff and then at the skip label some common stuff... Another thing you could consider is to have common functionality carved out into helpers with "common" in the name and then call those from both UMC and DCT paths. Perhaps that'll help keep the init paths sane. That is, short of splitting this driver. We did the splitting for Intel and there we have a common, librarized code which gets linked into a couple of drivers. You don't have to do this too - just putting it out there as an alternative. The per-family function pointers design could be good too, if done right. The advantage being, you have a single driver for all, yadda yadda... > Prepare for this by adding prep_chip_selects() to pvt->ops and set it > as needed based on currently supported systems. > > Use a "umc" prefix for modern systems, since these use Unified Memory > Controllers (UMCs). > > Use a "dct" prefix for newly-defined legacy functions, since these > systems use DRAM Controllers (DCTs). > > Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com> > Signed-off-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com> What does Naveen's SOB mean here? Co-developed-by perhaps?
On Wed, May 18, 2022 at 10:10:05AM +0200, Borislav Petkov wrote: > On Mon, May 09, 2022 at 02:55:22PM +0000, Yazen Ghannam wrote: > > From: Muralidhara M K <muralidhara.mk@amd.com> > > > > GPU Nodes will need to set the number of available Chip Selects, i.e. > > Base and Mask values, differently than existing systems. A function > > pointer should be used rather than introduce another branching condition. > > Yeah, it looks to me like all that detection logic should be split > eventually. Looking at read_mc_regs(), it has > > if (pvt->umc) { > __read_mc_regs_df(pvt); > > goto skip; > } > > at the top, then a whole bunch of legacy stuff and then at the skip > label some common stuff... > > Another thing you could consider is to have common functionality carved > out into helpers with "common" in the name and then call those from both > UMC and DCT paths. Perhaps that'll help keep the init paths sane. That > is, short of splitting this driver. > > We did the splitting for Intel and there we have a common, librarized > code which gets linked into a couple of drivers. You don't have to do > this too - just putting it out there as an alternative. > > The per-family function pointers design could be good too, if done > right. The advantage being, you have a single driver for all, yadda > yadda... > Yep, I've actually considered splitting this driver. But I think at this point it'd be best to keep what we have, and then write a new driver if and when a major change happens in future platforms. > > Prepare for this by adding prep_chip_selects() to pvt->ops and set it > > as needed based on currently supported systems. > > > > Use a "umc" prefix for modern systems, since these use Unified Memory > > Controllers (UMCs). > > > > Use a "dct" prefix for newly-defined legacy functions, since these > > systems use DRAM Controllers (DCTs). > > > > Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com> > > Signed-off-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com> > > What does Naveen's SOB mean here? Co-developed-by perhaps? > Yes, that's right. Sorry I missed it. I'll include it in the next revision. Thanks, Yazen
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index 3d569290d4cf..f8cd89278753 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -1483,7 +1483,7 @@ 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 dct_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; @@ -1491,20 +1491,22 @@ static void prep_chip_selects(struct amd64_pvt *pvt) } 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 = pvt->flags.zn_regs_v2 ? 4 : 2; - } - } else { pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8; pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4; } } +static void umc_prep_chip_selects(struct amd64_pvt *pvt) +{ + int umc; + + for_each_umc(umc) { + pvt->csels[umc].b_cnt = 4; + pvt->csels[umc].m_cnt = pvt->flags.zn_regs_v2 ? 4 : 2; + } +} + static void read_umc_base_mask(struct amd64_pvt *pvt) { u32 umc_base_reg, umc_base_reg_sec; @@ -1563,8 +1565,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); @@ -3301,6 +3301,8 @@ static void read_mc_regs(struct amd64_pvt *pvt) } skip: + pvt->ops->prep_chip_selects(pvt); + read_dct_base_mask(pvt); determine_memory_type(pvt); @@ -3762,6 +3764,7 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci) static struct low_ops umc_ops = { .early_channel_count = umc_early_channel_count, .dbam_to_cs = umc_addr_mask_to_cs_size, + .prep_chip_selects = umc_prep_chip_selects, }; /* Use Family 16h versions for defaults and adjust as needed below. */ @@ -3769,6 +3772,7 @@ static struct low_ops dct_ops = { .early_channel_count = f1x_early_channel_count, .map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow, .dbam_to_cs = f16_dbam_to_chip_select, + .prep_chip_selects = dct_prep_chip_selects, }; static int per_family_init(struct amd64_pvt *pvt) diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h index a4a27208532c..0a7738df396f 100644 --- a/drivers/edac/amd64_edac.h +++ b/drivers/edac/amd64_edac.h @@ -468,6 +468,7 @@ struct low_ops { struct err_info *err); int (*dbam_to_cs)(struct amd64_pvt *pvt, u8 dct, unsigned int cs_mode, int cs_mask_nr); + void (*prep_chip_selects)(struct amd64_pvt *pvt); }; int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,