| Message ID | 20220228161354.54923-3-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:42PM +0530, Naveen Krishna Chatradhi wrote: > From: Muralidhara M K <muralimk@amd.com> > > Add function pointer for get_base_mask() in pvt->ops > and assign family specific get_base_mask() definitions appropriately. > The commit message should include why the change is needed and not just what the change is. A few of my patches have similar feedback, so it's fresh in my mind. :) > 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 | 22 ++++++++++++++++------ > drivers/edac/amd64_edac.h | 1 + > 2 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c > index b21f43a3ec98..985c59d23a20 100644 > --- a/drivers/edac/amd64_edac.c > +++ b/drivers/edac/amd64_edac.c > @@ -1570,11 +1570,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); > @@ -3287,7 +3282,9 @@ static void read_mc_regs(struct amd64_pvt *pvt) > } > > skip: > - read_dct_base_mask(pvt); > + prep_chip_selects(pvt); > + > + pvt->ops->get_base_mask(pvt); > > determine_memory_type(pvt); > edac_dbg(1, " DIMM type: %s\n", edac_mem_types[pvt->dram_type]); > @@ -3763,6 +3760,7 @@ static int per_family_init(struct amd64_pvt *pvt) > 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->get_base_mask = read_dct_base_mask; > break; > > case 0x10: > @@ -3772,6 +3770,7 @@ static int per_family_init(struct amd64_pvt *pvt) > pvt->ops->early_channel_count = f1x_early_channel_count; > 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; > break; > > case 0x15: > @@ -3796,6 +3795,7 @@ static int per_family_init(struct amd64_pvt *pvt) > } > pvt->ops->early_channel_count = f1x_early_channel_count; > pvt->ops->map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow; > + pvt->ops->get_base_mask = read_dct_base_mask; > break; > > case 0x16: > @@ -3811,6 +3811,7 @@ static int per_family_init(struct amd64_pvt *pvt) > pvt->ops->early_channel_count = f1x_early_channel_count; > 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; > break; > > case 0x17: > @@ -3840,6 +3841,7 @@ static int per_family_init(struct amd64_pvt *pvt) > case 0x18: > 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; > > if (pvt->fam == 0x18) { > pvt->ctl_name = "F18h"; > @@ -3875,6 +3877,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; The function pointer is get_base_mask() and the helper functions are read_{dct,umc}_base_mask(). I think the naming should be more consistent. Either read_base_mask()/read_{dct,umc}_base_mask() or get_base_mask()/get_{dct,umc}_base_mask(). > break; > > default: > @@ -3882,6 +3885,13 @@ static int per_family_init(struct amd64_pvt *pvt) > return -ENODEV; > } > > + /* ops required for all the families */ > + if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs || > + !pvt->ops->get_base_mask) { > + edac_dbg(1, "Common helper routines not defined.\n"); > + return -EFAULT; > + } > + I think it'd be clearer if we define an ops struct with default, "do nothing" functions and statically include this in struct pvt. These can then be overwritten in per_family_init(). I think this would avoid the need to check that each function is set and the need to dynamically allocate the ops struct. Thanks, Yazen
On Wed, Mar 23, 2022 at 05:33:18PM +0000, Yazen Ghannam wrote: > The commit message should include why the change is needed and not just what > the change is. A few of my patches have similar feedback, so it's fresh in my > mind. :) Good. :-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index b21f43a3ec98..985c59d23a20 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -1570,11 +1570,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); @@ -3287,7 +3282,9 @@ static void read_mc_regs(struct amd64_pvt *pvt) } skip: - read_dct_base_mask(pvt); + prep_chip_selects(pvt); + + pvt->ops->get_base_mask(pvt); determine_memory_type(pvt); edac_dbg(1, " DIMM type: %s\n", edac_mem_types[pvt->dram_type]); @@ -3763,6 +3760,7 @@ static int per_family_init(struct amd64_pvt *pvt) 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->get_base_mask = read_dct_base_mask; break; case 0x10: @@ -3772,6 +3770,7 @@ static int per_family_init(struct amd64_pvt *pvt) pvt->ops->early_channel_count = f1x_early_channel_count; 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; break; case 0x15: @@ -3796,6 +3795,7 @@ static int per_family_init(struct amd64_pvt *pvt) } pvt->ops->early_channel_count = f1x_early_channel_count; pvt->ops->map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow; + pvt->ops->get_base_mask = read_dct_base_mask; break; case 0x16: @@ -3811,6 +3811,7 @@ static int per_family_init(struct amd64_pvt *pvt) pvt->ops->early_channel_count = f1x_early_channel_count; 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; break; case 0x17: @@ -3840,6 +3841,7 @@ static int per_family_init(struct amd64_pvt *pvt) case 0x18: 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; if (pvt->fam == 0x18) { pvt->ctl_name = "F18h"; @@ -3875,6 +3877,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; break; default: @@ -3882,6 +3885,13 @@ static int per_family_init(struct amd64_pvt *pvt) return -ENODEV; } + /* ops required for all the families */ + if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs || + !pvt->ops->get_base_mask) { + edac_dbg(1, "Common helper routines not defined.\n"); + return -EFAULT; + } + return 0; } diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h index 1b2055af26b9..cf38367e3aa1 100644 --- a/drivers/edac/amd64_edac.h +++ b/drivers/edac/amd64_edac.h @@ -464,6 +464,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 (*get_base_mask)(struct amd64_pvt *pvt); }; int __amd64_read_pci_cfg_dword(struct pci_dev *pdev, int offset,