Subject: Re: [PATCH iwl-next 1/4] ice: Implement PTP support for E830 devices

From: Simon Horman <[email-masked]>
Date: Tue, 9 Jul 2024 20:47:27 +0100

On Tue, Jul 09, 2024 at 02:34:55PM +0200, Karol Kolacinski wrote:
> From: Michal Michalik <[email-masked]>
> 
> 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 <[email-masked]>
> Co-developed-by: Milena Olech <[email-masked]>
> Signed-off-by: Milena Olech <[email-masked]>
> Co-developed-by: Paul Greenwalt <[email-masked]>
> Signed-off-by: Paul Greenwalt <[email-masked]>
> Signed-off-by: Michal Michalik <[email-masked]>
> Co-developed-by: Karol Kolacinski <[email-masked]>
> Signed-off-by: Karol Kolacinski <[email-masked]>

...

> 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

...