| Message ID | 20220228161354.54923-4-nchatrad@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EDAC/amd64: move platform specific routines to pvt->ops | expand |
On Mon, Feb 28, 2022 at 09:43:43PM +0530, Naveen Krishna Chatradhi wrote: > From: Muralidhara M K <muralimk@amd.com> > > Add function pointer for prep_chip_selects() in pvt->ops and assign > family specific prep_chip_selects() definitions appropriately. > Please include the "why" also. > 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 | 68 ++++++++++++++++++++++++++++----------- > drivers/edac/amd64_edac.h | 1 + > 2 files changed, 50 insertions(+), 19 deletions(-) > > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c > index 985c59d23a20..708c4bbc0d1c 100644 > --- a/drivers/edac/amd64_edac.c > +++ b/drivers/edac/amd64_edac.c > @@ -1490,28 +1490,51 @@ 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; > - } > + if (pvt->ext_model < K8_REV_F) { > + pvt->csels[0].b_cnt = 8; > + pvt->csels[1].b_cnt = 8; > + "b_cnt" is the same for both conditions, so these lines can be moved out of the if-else statements. > + pvt->csels[0].m_cnt = 8; > + pvt->csels[1].m_cnt = 8; > + } else if (pvt->ext_model >= K8_REV_F) { This can just be an "else". > + pvt->csels[0].b_cnt = 8; > + pvt->csels[1].b_cnt = 8; > + > + pvt->csels[0].m_cnt = 4; > + pvt->csels[1].m_cnt = 4; > + } > +} > > - } 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 f15_m30h_prep_chip_selects(struct amd64_pvt *pvt) > +{ > + pvt->csels[0].b_cnt = 4; > + pvt->csels[1].b_cnt = 4; > + > + pvt->csels[0].m_cnt = 2; > + pvt->csels[1].m_cnt = 2; Here, above, and below you can keep the single line style when setting variables to the same value. > +} > + > +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 default_prep_chip_selects(struct amd64_pvt *pvt) > +{ > + pvt->csels[0].b_cnt = 8; > + pvt->csels[1].b_cnt = 8; > + > + pvt->csels[0].m_cnt = 4; > + pvt->csels[1].m_cnt = 4; > +} > + This may be a good example of a default (though not "do nothing") function that can be set and overwritten when needed. That would save the NULL function pointer check and all the lines where the pointer gets set to this default function. > static void read_umc_base_mask(struct amd64_pvt *pvt) > { > u32 umc_base_reg, umc_base_reg_sec; > @@ -3282,7 +3305,7 @@ static void read_mc_regs(struct amd64_pvt *pvt) > } > > skip: > - prep_chip_selects(pvt); > + pvt->ops->prep_chip_selects(pvt); > > pvt->ops->get_base_mask(pvt); > > @@ -3761,6 +3784,7 @@ static int per_family_init(struct amd64_pvt *pvt) > pvt->ops->map_sysaddr_to_csrow = k8_map_sysaddr_to_csrow; > pvt->ops->dbam_to_cs = k8_dbam_to_chip_select; > pvt->ops->get_base_mask = read_dct_base_mask; > + pvt->ops->prep_chip_selects = k8_prep_chip_selects; > break; > > case 0x10: > @@ -3771,6 +3795,7 @@ static int per_family_init(struct amd64_pvt *pvt) > pvt->ops->map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow; > pvt->ops->dbam_to_cs = f10_dbam_to_chip_select; > pvt->ops->get_base_mask = read_dct_base_mask; > + pvt->ops->prep_chip_selects = default_prep_chip_selects; > break; > > case 0x15: > @@ -3779,11 +3804,13 @@ static int per_family_init(struct amd64_pvt *pvt) > pvt->f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1; > pvt->f2_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F2; > pvt->ops->dbam_to_cs = f16_dbam_to_chip_select; > + pvt->ops->prep_chip_selects = f15_m30h_prep_chip_selects; > } else if (pvt->model == 0x60) { > pvt->ctl_name = "F15h_M60h"; > pvt->f1_id = PCI_DEVICE_ID_AMD_15H_M60H_NB_F1; > pvt->f2_id = PCI_DEVICE_ID_AMD_15H_M60H_NB_F2; > pvt->ops->dbam_to_cs = f15_m60h_dbam_to_chip_select; > + pvt->ops->prep_chip_selects = default_prep_chip_selects; > } else if (pvt->model == 0x13) { > /* Richland is only client */ > return -ENODEV; > @@ -3812,6 +3839,7 @@ static int per_family_init(struct amd64_pvt *pvt) > pvt->ops->map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow; > pvt->ops->dbam_to_cs = f16_dbam_to_chip_select; > pvt->ops->get_base_mask = read_dct_base_mask; > + pvt->ops->prep_chip_selects = default_prep_chip_selects; > break; > > case 0x17: > @@ -3842,6 +3870,7 @@ static int per_family_init(struct amd64_pvt *pvt) > pvt->ops->early_channel_count = f17_early_channel_count; > pvt->ops->dbam_to_cs = f17_addr_mask_to_cs_size; > pvt->ops->get_base_mask = read_umc_base_mask; > + pvt->ops->prep_chip_selects = f17_prep_chip_selects; > > if (pvt->fam == 0x18) { > pvt->ctl_name = "F18h"; > @@ -3878,6 +3907,7 @@ static int per_family_init(struct amd64_pvt *pvt) > pvt->ops->early_channel_count = f17_early_channel_count; > pvt->ops->dbam_to_cs = f17_addr_mask_to_cs_size; > pvt->ops->get_base_mask = read_umc_base_mask; > + pvt->ops->prep_chip_selects = f17_prep_chip_selects; > break; > I expect that all the Zen-based CPU ops will be the same. Also, I figure that Zen-based CPUs are likely the majority of AMD-based systems in use today, or at least those that will use updated kernel versions. So I think that the Zen-based CPU ops should be the default. GPU and legacy CPU ops can be set as needed during init time. Thanks, Yazen
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index 985c59d23a20..708c4bbc0d1c 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -1490,28 +1490,51 @@ 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; - } + if (pvt->ext_model < K8_REV_F) { + pvt->csels[0].b_cnt = 8; + pvt->csels[1].b_cnt = 8; + + pvt->csels[0].m_cnt = 8; + pvt->csels[1].m_cnt = 8; + } else if (pvt->ext_model >= K8_REV_F) { + pvt->csels[0].b_cnt = 8; + pvt->csels[1].b_cnt = 8; + + pvt->csels[0].m_cnt = 4; + pvt->csels[1].m_cnt = 4; + } +} - } 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 f15_m30h_prep_chip_selects(struct amd64_pvt *pvt) +{ + pvt->csels[0].b_cnt = 4; + pvt->csels[1].b_cnt = 4; + + pvt->csels[0].m_cnt = 2; + pvt->csels[1].m_cnt = 2; +} + +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 default_prep_chip_selects(struct amd64_pvt *pvt) +{ + pvt->csels[0].b_cnt = 8; + pvt->csels[1].b_cnt = 8; + + pvt->csels[0].m_cnt = 4; + pvt->csels[1].m_cnt = 4; +} + static void read_umc_base_mask(struct amd64_pvt *pvt) { u32 umc_base_reg, umc_base_reg_sec; @@ -3282,7 +3305,7 @@ static void read_mc_regs(struct amd64_pvt *pvt) } skip: - prep_chip_selects(pvt); + pvt->ops->prep_chip_selects(pvt); pvt->ops->get_base_mask(pvt); @@ -3761,6 +3784,7 @@ static int per_family_init(struct amd64_pvt *pvt) pvt->ops->map_sysaddr_to_csrow = k8_map_sysaddr_to_csrow; pvt->ops->dbam_to_cs = k8_dbam_to_chip_select; pvt->ops->get_base_mask = read_dct_base_mask; + pvt->ops->prep_chip_selects = k8_prep_chip_selects; break; case 0x10: @@ -3771,6 +3795,7 @@ static int per_family_init(struct amd64_pvt *pvt) pvt->ops->map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow; pvt->ops->dbam_to_cs = f10_dbam_to_chip_select; pvt->ops->get_base_mask = read_dct_base_mask; + pvt->ops->prep_chip_selects = default_prep_chip_selects; break; case 0x15: @@ -3779,11 +3804,13 @@ static int per_family_init(struct amd64_pvt *pvt) pvt->f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1; pvt->f2_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F2; pvt->ops->dbam_to_cs = f16_dbam_to_chip_select; + pvt->ops->prep_chip_selects = f15_m30h_prep_chip_selects; } else if (pvt->model == 0x60) { pvt->ctl_name = "F15h_M60h"; pvt->f1_id = PCI_DEVICE_ID_AMD_15H_M60H_NB_F1; pvt->f2_id = PCI_DEVICE_ID_AMD_15H_M60H_NB_F2; pvt->ops->dbam_to_cs = f15_m60h_dbam_to_chip_select; + pvt->ops->prep_chip_selects = default_prep_chip_selects; } else if (pvt->model == 0x13) { /* Richland is only client */ return -ENODEV; @@ -3812,6 +3839,7 @@ static int per_family_init(struct amd64_pvt *pvt) pvt->ops->map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow; pvt->ops->dbam_to_cs = f16_dbam_to_chip_select; pvt->ops->get_base_mask = read_dct_base_mask; + pvt->ops->prep_chip_selects = default_prep_chip_selects; break; case 0x17: @@ -3842,6 +3870,7 @@ static int per_family_init(struct amd64_pvt *pvt) pvt->ops->early_channel_count = f17_early_channel_count; pvt->ops->dbam_to_cs = f17_addr_mask_to_cs_size; pvt->ops->get_base_mask = read_umc_base_mask; + pvt->ops->prep_chip_selects = f17_prep_chip_selects; if (pvt->fam == 0x18) { pvt->ctl_name = "F18h"; @@ -3878,6 +3907,7 @@ static int per_family_init(struct amd64_pvt *pvt) pvt->ops->early_channel_count = f17_early_channel_count; pvt->ops->dbam_to_cs = f17_addr_mask_to_cs_size; pvt->ops->get_base_mask = read_umc_base_mask; + pvt->ops->prep_chip_selects = f17_prep_chip_selects; break; default: @@ -3887,7 +3917,7 @@ 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->get_base_mask || !pvt->ops->prep_chip_selects) { edac_dbg(1, "Common helper routines not defined.\n"); return -EFAULT; } diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h index cf38367e3aa1..cca59a1b3021 100644 --- a/drivers/edac/amd64_edac.h +++ b/drivers/edac/amd64_edac.h @@ -465,6 +465,7 @@ struct low_ops { int (*dbam_to_cs)(struct amd64_pvt *pvt, u8 dct, unsigned int cs_mode, int cs_mask_nr); void (*get_base_mask)(struct amd64_pvt *pvt); + void (*prep_chip_selects)(struct amd64_pvt *pvt); }; int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,