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

[Simon Horman <horms@kernel.org>]

On Tue, Jul 09, 2024 at 02:34:55PM +0200, Karol Kolacinski wrote:
> From: Michal Michalik <michal.michalik@intel.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@intel.com>
> Co-developed-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Milena Olech <milena.olech@intel.com>
> Co-developed-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Michal Michalik <michal.michalik@intel.com>
> Co-developed-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.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

...