netdev - Re: [Intel-wired-lan] [PATCH v2 iwl-next 03/10] ice: use designated initializers for TSPLL consts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <addf238e-56ab-44b7-87c6-f173df1223f3@intel.com>
Date: Thu, 17 Apr 2025 17:06:17 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Karol Kolacinski <karol.kolacinski@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, <anthony.l.nguyen@...el.com>,
	<przemyslaw.kitszel@...el.com>, Michal Kubiak <michal.kubiak@...el.com>,
	Milena Olech <milena.olech@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH v2 iwl-next 03/10] ice: use designated
 initializers for TSPLL consts



On 4/9/2025 5:25 AM, Karol Kolacinski wrote:
> Instead of multiple comments, use designated initializers for TSPLL
> consts.
> 
> Adjust ice_tspll_params_e82x fields sizes.
> 
> Remove ice_tspll_params_e825 definitions as according to EDS (Electrical
> Design Specification) doc, E825 devices support only 156.25 MHz TSPLL
> frequency for both TCXO and TIME_REF clock source.
> 
> Reviewed-by: Michal Kubiak <michal.kubiak@...el.com>
> Reviewed-by: Milena Olech <milena.olech@...el.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@...el.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_common.h |  19 +-
>  drivers/net/ethernet/intel/ice/ice_tspll.c  | 203 ++++----------------
>  drivers/net/ethernet/intel/ice/ice_tspll.h  |  29 +--
>  3 files changed, 64 insertions(+), 187 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
> index ce5f561fc481..83e991c160ba 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.h
> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
> @@ -74,11 +74,11 @@ union ice_cgu_r16 {
>  };
>  
>  #define ICE_CGU_R19 0x4c
> -union ice_cgu_r19 {
> +union ice_cgu_r19_e82x {
>  	struct {
>  		u32 fbdiv_intgr : 8;
>  		u32 fdpll_ulck_thr : 5;
> -		u32 misc15 : 3;
> +		u32 misc15 : 1;
>  		u32 ndivratio : 4;

This change appears to be a copy mistake. It will result in the offset
for ndivratio being incorrect at BIT(14) instead of BIT(16). You later
correct this in the series when you convert to bitmasks, but this should
be removed from this committ.

>  		u32 tspll_iref_ndivratio : 3;
>  		u32 misc19 : 1;
> @@ -89,6 +89,21 @@ union ice_cgu_r19 {
>  	u32 val;
>  };
>  
> +union ice_cgu_r19_e825 {
> +	struct {
> +		u32 tspll_fbdiv_intgr : 10;
> +		u32 fdpll_ulck_thr : 5;
> +		u32 misc15 : 1;
> +		u32 tspll_ndivratio : 4;
> +		u32 tspll_iref_ndivratio : 3;
> +		u32 misc19 : 1;
> +		u32 japll_ndivratio : 4;
> +		u32 japll_postdiv_pdivratio : 3;
> +		u32 misc27 : 1;
> +	};
> +	u32 val;
> +};
> +

In fact, I believe this entire change should be separated to its own
commit, along with the other functional changes made in the conversion
to bitmask.


> -static const struct
> -ice_tspll_params_e825c e825c_tspll_params[NUM_ICE_TSPLL_FREQ] = {
> -	/* ICE_TSPLL_FREQ_25_000 -> 25 MHz */
> -	{
> -		/* ck_refclkfreq */
> -		0x19,
> -		/* ndivratio */
> -		1,
> -		/* fbdiv_intgr */
> -		320,
> -		/* fbdiv_frac */
> -		0,
> -		/* ref1588_ck_div */
> -		0,
> +	[ICE_TSPLL_FREQ_122_880] = {
> +		.refclk_pre_div = 5,
> +		.post_pll_div = 7,
> +		.feedback_div = 223,
> +		.frac_n_div = 524288
>  	},
> -
> -	/* ICE_TSPLL_FREQ_122_880 -> 122.88 MHz */
> -	{
> -		/* ck_refclkfreq */
> -		0x29,
> -		/* ndivratio */
> -		3,
> -		/* fbdiv_intgr */
> -		195,
> -		/* fbdiv_frac */
> -		1342177280UL,
> -		/* ref1588_ck_div */
> -		0,
> +	[ICE_TSPLL_FREQ_125_000] = {
> +		.refclk_pre_div = 5,
> +		.post_pll_div = 7,
> +		.feedback_div = 223,
> +		.frac_n_div = 524288
>  	},
> -
> -	/* ICE_TSPLL_FREQ_125_000 -> 125 MHz */
> -	{
> -		/* ck_refclkfreq */
> -		0x3E,
> -		/* ndivratio */
> -		2,
> -		/* fbdiv_intgr */
> -		128,
> -		/* fbdiv_frac */
> -		0,
> -		/* ref1588_ck_div */
> -		0,
> +	[ICE_TSPLL_FREQ_153_600] = {
> +		.refclk_pre_div = 5,
> +		.post_pll_div = 6,
> +		.feedback_div = 159,
> +		.frac_n_div = 1572864
>  	},
> -
> -	/* ICE_TSPLL_FREQ_153_600 -> 153.6 MHz */
> -	{
> -		/* ck_refclkfreq */
> -		0x33,
> -		/* ndivratio */
> -		3,
> -		/* fbdiv_intgr */
> -		156,
> -		/* fbdiv_frac */
> -		1073741824UL,
> -		/* ref1588_ck_div */
> -		0,
> -	},
> -
> -	/* ICE_TSPLL_FREQ_156_250 -> 156.25 MHz */
> -	{
> -		/* ck_refclkfreq */
> -		0x1F,
> -		/* ndivratio */
> -		5,
> -		/* fbdiv_intgr */
> -		256,
> -		/* fbdiv_frac */
> -		0,
> -		/* ref1588_ck_div */
> -		0,
> -	},
> -
> -	/* ICE_TSPLL_FREQ_245_760 -> 245.76 MHz */
> -	{
> -		/* ck_refclkfreq */
> -		0x52,
> -		/* ndivratio */
> -		3,
> -		/* fbdiv_intgr */
> -		97,
> -		/* fbdiv_frac */
> -		2818572288UL,
> -		/* ref1588_ck_div */
> -		0,
> +	[ICE_TSPLL_FREQ_156_250] = {
> +		.refclk_pre_div = 5,
> +		.post_pll_div = 6,
> +		.feedback_div = 159,
> +		.frac_n_div = 1572864
>  	},
> +	[ICE_TSPLL_FREQ_245_760] = {
> +		.refclk_pre_div = 10,
> +		.post_pll_div = 7,
> +		.feedback_div = 223,
> +		.frac_n_div = 524288
> +	}
>  };
>  

The code diff is also a lot cleaner if you split the removal of the
E825-C array as a separate commit. Otherwise, this noisy diff is much
harder to process.