netdev - Re: [Intel-wired-lan] [PATCH v3 iwl-next 1/4] ice: Implement PTP support for E830 devices

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <5fb6f2ad-6db1-4dd6-b985-19dd14035603@intel.com>
Date: Fri, 26 Jul 2024 15:17:20 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Karol Kolacinski <karol.kolacinski@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, Paul Greenwalt
	<paul.greenwalt@...el.com>, Michal Michalik <michal.michalik@...el.com>,
	<netdev@...r.kernel.org>, <anthony.l.nguyen@...el.com>,
	<przemyslaw.kitszel@...el.com>, Milena Olech <milena.olech@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH v3 iwl-next 1/4] ice: Implement PTP
 support for E830 devices

From: Karol Kolacinski <karol.kolacinski@...el.com>
Date: Thu, 25 Jul 2024 11:34:48 +0200

> From: Michal Michalik <michal.michalik@...el.com>
> 
> Add specific functions and definitions for E830 devices to enable
> PTP support.
> Introduce new PHY model ICE_PHY_E830.
> E830 devices support direct write to GLTSYN_ registers without shadow
> registers and 64 bit read of PHC time.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Co-developed-by: Milena Olech <milena.olech@...el.com>
> Signed-off-by: Milena Olech <milena.olech@...el.com>
> Co-developed-by: Paul Greenwalt <paul.greenwalt@...el.com>
> Signed-off-by: Paul Greenwalt <paul.greenwalt@...el.com>
> Signed-off-by: Michal Michalik <michal.michalik@...el.com>
> Co-developed-by: Karol Kolacinski <karol.kolacinski@...el.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@...el.com>
> ---
> V2 -> V3: Fixed kdoc for ice_is_e***() and ice_ptp_init_phy_e830()
> V1 -> V2: Fixed compilation issue with GENMASK bits higher than 32
> 
>  drivers/net/ethernet/intel/ice/ice_common.c   |  13 +-
>  drivers/net/ethernet/intel/ice/ice_common.h   |   1 +
>  .../net/ethernet/intel/ice/ice_hw_autogen.h   |   4 +
>  drivers/net/ethernet/intel/ice/ice_ptp.c      |  11 +-
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 199 ++++++++++++++++--
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  25 ++-
>  drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
>  7 files changed, 226 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 009716a12a26..8bff63d3d664 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -267,7 +267,7 @@ bool ice_is_e822(struct ice_hw *hw)
>   * ice_is_e823
>   * @hw: pointer to the hardware structure
>   *
> - * returns true if the device is E823-L or E823-C based, false if not.
> + * Return: true if the device is E823-L or E823-C based, false if not.

I'd move this, together with FIELD_{GET,PREP}() conversion, to a
separate commit maybe (one is enough).

>   */
>  bool ice_is_e823(struct ice_hw *hw)
>  {
> @@ -307,6 +307,17 @@ bool ice_is_e825c(struct ice_hw *hw)
>  	}
>  }
>  
> +/**
> + * ice_is_e830
> + * @hw: pointer to the hardware structure
> + *
> + * Return: true if the device is E830 based, false if not.
> + */
> +bool ice_is_e830(const struct ice_hw *hw)
> +{
> +	return hw->mac_type == ICE_MAC_E830;
> +}

I think making this one check external instead of a static inline only
increases object code size. Can't they reside in the header?

[...]

> +/**
> + * ice_ptp_init_phc_e830 - Perform E830 specific PHC initialization
> + * @hw: pointer to HW struct
> + *
> + * Perform E830-specific PTP hardware clock initialization steps.
> + *
> + * Return: 0 on success
> + */
> +static int ice_ptp_init_phc_e830(struct ice_hw *hw)
> +{
> +	ice_ptp_cfg_sync_delay(hw, ICE_E810_E830_SYNC_DELAY);
> +	return 0;

Can't this and the below functions be void since they always return zero?

> +}
> +
> +/**
> + * ice_ptp_write_direct_incval_e830 - Prep PHY port increment value change
> + * @hw: pointer to HW struct
> + * @incval: The new 40bit increment value to prepare
> + *
> + * Prepare the PHY port for a new increment value by programming the PHC
> + * GLTSYN_INCVAL_L and GLTSYN_INCVAL_H registers. The actual change is
> + * completed by FW automatically.
> + */
> +static int ice_ptp_write_direct_incval_e830(struct ice_hw *hw, u64 incval)

const @hw (below as well)?

[...]

> @@ -5474,12 +5618,14 @@ void ice_ptp_init_hw(struct ice_hw *hw)
>  {
>  	struct ice_ptp_hw *ptp = &hw->ptp;
>  
> -	if (ice_is_e822(hw) || ice_is_e823(hw))
> -		ice_ptp_init_phy_e82x(ptp);
> -	else if (ice_is_e810(hw))
> +	if (ice_is_e810(hw))
>  		ice_ptp_init_phy_e810(ptp);
> +	else if (ice_is_e822(hw) || ice_is_e823(hw))
> +		ice_ptp_init_phy_e82x(ptp);
>  	else if (ice_is_e825c(hw))
>  		ice_ptp_init_phy_e825c(hw);
> +	else if (ice_is_e830(hw))
> +		ice_ptp_init_phy_e830(ptp);
>  	else
>  		ptp->phy_model = ICE_PHY_UNSUP;

Since at least most of these functions just check for one field, can't
this be one switch-case on that fields (if you open-code that check)?

>  }
> @@ -5570,6 +5716,8 @@ static int ice_ptp_port_cmd(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd)
>  	switch (hw->ptp.phy_model) {
>  	case ICE_PHY_E810:
>  		return ice_ptp_port_cmd_e810(hw, cmd);
> +	case ICE_PHY_E830:
> +		return ice_ptp_port_cmd_e830(hw, cmd);
>  	default:
>  		break;
>  	}
> @@ -5640,6 +5788,10 @@ int ice_ptp_init_time(struct ice_hw *hw, u64 time)
>  	tmr_idx = hw->func_caps.ts_func_info.tmr_index_owned;
>  
>  	/* Source timers */
> +	/* For E830 we don't need to use shadow registers, its automatic */

							   ^^^
							   it's

[...]

Thanks,
Olek