netdev - Re: [PATCH 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: <20240709194727.GP346094@kernel.org>
Date: Tue, 9 Jul 2024 20:47:27 +0100
From: Simon Horman <horms@...nel.org>
To: Karol Kolacinski <karol.kolacinski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com,
	Michal Michalik <michal.michalik@...el.com>,
	Milena Olech <milena.olech@...el.com>,
	Paul Greenwalt <paul.greenwalt@...el.com>
Subject: Re: [PATCH iwl-next 1/4] ice: Implement PTP support for E830 devices

On Tue, Jul 09, 2024 at 02:34:55PM +0200, Karol Kolacinski wrote:
> 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>

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 009716a12a26..005054439204 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -307,6 +307,17 @@ bool ice_is_e825c(struct ice_hw *hw)
>  	}
>  }
>  
> +/**
> + * ice_is_e830
> + * @hw: pointer to the hardware structure
> + *
> + * returns true if the device is E830 based, false if not.

Hi Michal, Karol, all,

Please consider documenting return values using a "Return:" or "Returns:"
section.

Flagged by: kernel-doc -none -Wall

> + */
> +bool ice_is_e830(const struct ice_hw *hw)
> +{
> +	return hw->mac_type == ICE_MAC_E830;
> +}
> +
>  /**
>   * ice_clear_pf_cfg - Clear PF configuration
>   * @hw: pointer to the hardware structure

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> index 06500028c760..3a5dd65a9a80 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> @@ -327,6 +327,7 @@ extern const struct ice_vernier_info_e82x e822_vernier[NUM_ICE_PTP_LNK_SPD];
>  #define ICE_E810_PLL_FREQ		812500000
>  #define ICE_PTP_NOMINAL_INCVAL_E810	0x13b13b13bULL
>  #define ICE_E810_OUT_PROP_DELAY_NS	1
> +#define ICE_E810_E830_SYNC_DELAY	0
>  #define ICE_E825C_OUT_PROP_DELAY_NS	11
>  
>  /* Device agnostic functions */
> @@ -673,18 +674,21 @@ static inline bool ice_is_primary(struct ice_hw *hw)
>  /* E810 timer command register */
>  #define E810_ETH_GLTSYN_CMD		0x03000344
>  
> +/* E830 timer command register */
> +#define E830_ETH_GLTSYN_CMD		0x00088814
> +
> +/* E810 PHC time register */
> +#define E830_GLTSYN_TIME_L(_tmr_idx)	(0x0008A000 + 0x1000 * (_tmr_idx))
> +
>  /* Source timer incval macros */
>  #define INCVAL_HIGH_M			0xFF
>  
> -/* Timestamp block macros */
> +/* PHY 40b registers macros */
> +#define PHY_EXT_40B_LOW_M		GENMASK(31, 0)
> +#define PHY_EXT_40B_HIGH_M		GENMASK(39, 32)
> +#define PHY_40B_LOW_M			GENMASK(7, 0)
> +#define PHY_40B_HIGH_M			GENMASK(39, 8)

I think that GENMASK_ULL needs to be used here
to avoid breakage on systems with 32bit unsigned long.

>  #define TS_VALID			BIT(0)
> -#define TS_LOW_M			0xFFFFFFFF
> -#define TS_HIGH_M			0xFF
> -#define TS_HIGH_S			32
> -
> -#define TS_PHY_LOW_M			0xFF
> -#define TS_PHY_HIGH_M			0xFFFFFFFF
> -#define TS_PHY_HIGH_S			8
>  
>  #define BYTES_PER_IDX_ADDR_L_U		8
>  #define BYTES_PER_IDX_ADDR_L		4

...