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

[Yazen Ghannam <yazen.ghannam@amd.com>]

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