Re: [Intel-wired-lan] [PATCH v4 iwl-next 02/12] ice: Introduce helper to get tmr_cmd_reg values

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 3/29/2024 9:09 AM, Karol Kolacinski wrote:
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> Multiple places in the driver code need to convert enum ice_ptp_tmr_cmd
> values into register bits for both the main timer and the PHY port
> timers. The main MAC register has one bit scheme for timer commands,
> while the PHY commands use a different scheme.
> 
> The E810 and E830 devices use the same scheme for port commands as used
> for the main timer. However, E822 and ETH56G hardware has a separate
> scheme used by the PHY.
> 
> Introduce helper functions to convert the timer command enumeration into
> the register values, reducing some code duplication, and making it
> easier to later refactor the individual port write commands.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 140 ++++++++++++--------
>   drivers/net/ethernet/intel/ice/ice_ptp_hw.h |   2 +-
>   2 files changed, 89 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index e86ca6cada79..c892b966c3b8 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -227,40 +227,114 @@ static u64 ice_ptp_read_src_incval(struct ice_hw *hw)
>   }
>   
>   /**
> - * ice_ptp_src_cmd - Prepare source timer for a timer command
> - * @hw: pointer to HW structure
> + * ice_ptp_tmr_cmd_to_src_reg - Convert to source timer command value
> + * @hw: pointer to HW struct
>    * @cmd: Timer command
>    *
> - * Prepare the source timer for an upcoming timer sync command.
> + * Returns: the source timer command register value for the given PTP timer
> + * command.
>    */
> -void ice_ptp_src_cmd(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd)
> +static u32 ice_ptp_tmr_cmd_to_src_reg(struct ice_hw *hw,
> +				      enum ice_ptp_tmr_cmd cmd)
>   {
> -	u32 cmd_val;
> -	u8 tmr_idx;
> +	u32 cmd_val, tmr_idx;
> +
> +	switch (cmd) {
> +	case ICE_PTP_INIT_TIME:
> +		cmd_val = GLTSYN_CMD_INIT_TIME;
> +		break;
> +	case ICE_PTP_INIT_INCVAL:
> +		cmd_val = GLTSYN_CMD_INIT_INCVAL;
> +		break;
> +	case ICE_PTP_ADJ_TIME:
> +		cmd_val = GLTSYN_CMD_ADJ_TIME;
> +		break;
> +	case ICE_PTP_ADJ_TIME_AT_TIME:
> +		cmd_val = GLTSYN_CMD_ADJ_INIT_TIME;
> +		break;
> +	case ICE_PTP_NOP:
> +	case ICE_PTP_READ_TIME:
> +		cmd_val = GLTSYN_CMD_READ_TIME;
> +		break;
> +	default:
> +		dev_warn(ice_hw_to_dev(hw),
> +			 "Ignoring unrecognized timer command %u\n", cmd);
> +		cmd_val = 0;
> +	}
>   
>   	tmr_idx = ice_get_ptp_src_clock_index(hw);
> -	cmd_val = tmr_idx << SEL_CPK_SRC;
> +
> +	return tmr_idx | cmd_val << SEL_CPK_SRC;

This is not equivalent to what was returned before this patch. Is this 
supposed to return different values now?

> +}
> +
> +/**
> + * ice_ptp_tmr_cmd_to_port_reg- Convert to port timer command value
> + * @hw: pointer to HW struct
> + * @cmd: Timer command
> + *
> + * Note that some hardware families use a different command register value for
> + * the PHY ports, while other hardware families use the same register values
> + * as the source timer.
> + *
> + * Returns: the PHY port timer command register value for the given PTP timer
> + * command.
> + */
> +static u32 ice_ptp_tmr_cmd_to_port_reg(struct ice_hw *hw,
> +				       enum ice_ptp_tmr_cmd cmd)
> +{
> +	u32 cmd_val, tmr_idx;
> +
> +	/* Certain hardware families share the same register values for the
> +	 * port register and source timer register.
> +	 */
> +	switch (hw->ptp.phy_model) {
> +	case ICE_PHY_E810:
> +		return ice_ptp_tmr_cmd_to_src_reg(hw, cmd) & TS_CMD_MASK_E810;
> +	default:
> +		break;
> +	}
>   
>   	switch (cmd) {
>   	case ICE_PTP_INIT_TIME:
> -		cmd_val |= GLTSYN_CMD_INIT_TIME;
> +		cmd_val = PHY_CMD_INIT_TIME;
>   		break;
>   	case ICE_PTP_INIT_INCVAL:
> -		cmd_val |= GLTSYN_CMD_INIT_INCVAL;
> +		cmd_val = PHY_CMD_INIT_INCVAL;
>   		break;
>   	case ICE_PTP_ADJ_TIME:
> -		cmd_val |= GLTSYN_CMD_ADJ_TIME;
> +		cmd_val = PHY_CMD_ADJ_TIME;
>   		break;
>   	case ICE_PTP_ADJ_TIME_AT_TIME:
> -		cmd_val |= GLTSYN_CMD_ADJ_INIT_TIME;
> +		cmd_val = PHY_CMD_ADJ_TIME_AT_TIME;
>   		break;
>   	case ICE_PTP_READ_TIME:
> -		cmd_val |= GLTSYN_CMD_READ_TIME;
> +		cmd_val = PHY_CMD_READ_TIME;
>   		break;
>   	case ICE_PTP_NOP:
> +		cmd_val = 0;
>   		break;
> +	default:
> +		dev_warn(ice_hw_to_dev(hw),
> +			 "Ignoring unrecognized timer command %u\n", cmd);
> +		cmd_val = 0;
>   	}
>   
> +	tmr_idx = ice_get_ptp_src_clock_index(hw);
> +
> +	return tmr_idx | cmd_val << SEL_PHY_SRC;

Just pointing out that this is the same as the above function in case 
the previous needs to be changed.

> +}
> +