[PATCH net-next 08/11] ice: add admin commands to access cgu configuration

[Simon Horman]

On Thu, Jul 20, 2023 at 10:19:00AM +0100, Vadim Fedorenko wrote:

...

Hi Vadim,

> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c

...

> +/**
> + * ice_aq_get_cgu_dpll_status
> + * @hw: pointer to the HW struct
> + * @dpll_num: DPLL index
> + * @ref_state: Reference clock state
> + * @dpll_state: DPLL state

./scripts/kernel-doc says that @config is missing here.

> + * @phase_offset: Phase offset in ns
> + * @eec_mode: EEC_mode
> + *
> + * Get CGU DPLL status (0x0C66)
> + */
> +int
> +ice_aq_get_cgu_dpll_status(struct ice_hw *hw, u8 dpll_num, u8 *ref_state,
> +			   u8 *dpll_state, u8 *config, s64 *phase_offset,
> +			   u8 *eec_mode)

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

...

> +/**
> + * ice_get_cgu_state - get the state of the DPLL
> + * @hw: pointer to the hw struct
> + * @dpll_idx: Index of internal DPLL unit
> + * @last_dpll_state: last known state of DPLL
> + * @pin: pointer to a buffer for returning currently active pin
> + * @ref_state: reference clock state

Likewise, @eec_mode is missing here.

> + * @phase_offset: pointer to a buffer for returning phase offset
> + * @dpll_state: state of the DPLL (output)

And @mode is missing here.

> + *
> + * This function will read the state of the DPLL(dpll_idx). Non-null
> + * 'pin', 'ref_state', 'eec_mode' and 'phase_offset' parameters are used to
> + * retrieve currently active pin, state, mode and phase_offset respectively.
> + *
> + * Return: state of the DPLL
> + */
> +int ice_get_cgu_state(struct ice_hw *hw, u8 dpll_idx,
> +		      enum dpll_lock_status last_dpll_state, u8 *pin,
> +		      u8 *ref_state, u8 *eec_mode, s64 *phase_offset,
> +		      enum dpll_lock_status *dpll_state,
> +		      enum dpll_mode *mode)
> +{
> +	u8 hw_ref_state, hw_dpll_state, hw_eec_mode, hw_config;
> +	s64 hw_phase_offset;
> +	int status;
> +
> +	status = ice_aq_get_cgu_dpll_status(hw, dpll_idx, &hw_ref_state,
> +					    &hw_dpll_state, &hw_config,
> +					    &hw_phase_offset, &hw_eec_mode);
> +	if (status) {
> +		*dpll_state = ICE_CGU_STATE_INVALID;

dpll_state is of type enum dpll_lock_status.
But the type of ICE_CGU_STATE_INVALID is enum ice_cgu_state.
Is this intended?

As flagged by gcc-12 W=1 and clang-16 W=1 builds.

> +		return status;
> +	}
> +
> +	if (pin)
> +		/* current ref pin in dpll_state_refsel_status_X register */
> +		*pin = hw_config & ICE_AQC_GET_CGU_DPLL_CONFIG_CLK_REF_SEL;
> +	if (phase_offset)
> +		*phase_offset = hw_phase_offset;
> +	if (ref_state)
> +		*ref_state = hw_ref_state;
> +	if (eec_mode)
> +		*eec_mode = hw_eec_mode;
> +	if (!dpll_state)
> +		return status;

Here dpll_state is checked for NULL.
But, above, it is dereferenced in the case where ice_aq_get_cgu_dpll_status
fails. Is that safe?

Also, perhaps it makes things a bit clearer to return 0 here.

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h

...

> +static const struct ice_cgu_pin_desc ice_e810t_sfp_cgu_inputs[] = {
> +	{ "CVL-SDP22",	  ZL_REF0P, DPLL_PIN_TYPE_INT_OSCILLATOR,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "CVL-SDP20",	  ZL_REF0N, DPLL_PIN_TYPE_INT_OSCILLATOR,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "C827_0-RCLKA", ZL_REF1P, DPLL_PIN_TYPE_MUX, 0, },
> +	{ "C827_0-RCLKB", ZL_REF1N, DPLL_PIN_TYPE_MUX, 0, },
> +	{ "SMA1",	  ZL_REF3P, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "SMA2/U.FL2",	  ZL_REF3N, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "GNSS-1PPS",	  ZL_REF4P, DPLL_PIN_TYPE_GNSS,
> +		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +	{ "OCXO",	  ZL_REF4N, DPLL_PIN_TYPE_INT_OSCILLATOR, 0, },
> +};

A gcc-12 W=1 build warns that ice_e810t_sfp_cgu_inputs, and
the similar static variables below, are unused when ice_ptp_hw.h
is included in ice_main.c via ice.h.

Looking at ice_e823_zl_cgu_outputs[], it seems to only be used
in ice_ptp_hw.c, so perhaps it could be defined there.

Perhaps that is also true of the other static variables below,
but I didn't check that.

> +
> +static const struct ice_cgu_pin_desc ice_e810t_qsfp_cgu_inputs[] = {
> +	{ "CVL-SDP22",	  ZL_REF0P, DPLL_PIN_TYPE_INT_OSCILLATOR,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "CVL-SDP20",	  ZL_REF0N, DPLL_PIN_TYPE_INT_OSCILLATOR,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "C827_0-RCLKA", ZL_REF1P, DPLL_PIN_TYPE_MUX, },
> +	{ "C827_0-RCLKB", ZL_REF1N, DPLL_PIN_TYPE_MUX, },
> +	{ "C827_1-RCLKA", ZL_REF2P, DPLL_PIN_TYPE_MUX, },
> +	{ "C827_1-RCLKB", ZL_REF2N, DPLL_PIN_TYPE_MUX, },
> +	{ "SMA1",	  ZL_REF3P, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "SMA2/U.FL2",	  ZL_REF3N, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "GNSS-1PPS",	  ZL_REF4P, DPLL_PIN_TYPE_GNSS,
> +		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +	{ "OCXO",	  ZL_REF4N, DPLL_PIN_TYPE_INT_OSCILLATOR, },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e810t_sfp_cgu_outputs[] = {
> +	{ "REF-SMA1",	    ZL_OUT0, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "REF-SMA2/U.FL2", ZL_OUT1, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "PHY-CLK",	    ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, },
> +	{ "MAC-CLK",	    ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, },
> +	{ "CVL-SDP21",	    ZL_OUT4, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +	{ "CVL-SDP23",	    ZL_OUT5, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e810t_qsfp_cgu_outputs[] = {
> +	{ "REF-SMA1",	    ZL_OUT0, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "REF-SMA2/U.FL2", ZL_OUT1, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "PHY-CLK",	    ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> +	{ "PHY2-CLK",	    ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> +	{ "MAC-CLK",	    ZL_OUT4, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> +	{ "CVL-SDP21",	    ZL_OUT5, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +	{ "CVL-SDP23",	    ZL_OUT6, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e823_si_cgu_inputs[] = {
> +	{ "NONE",	  SI_REF0P, 0, 0 },
> +	{ "NONE",	  SI_REF0N, 0, 0 },
> +	{ "SYNCE0_DP",	  SI_REF1P, DPLL_PIN_TYPE_MUX, 0 },
> +	{ "SYNCE0_DN",	  SI_REF1N, DPLL_PIN_TYPE_MUX, 0 },
> +	{ "EXT_CLK_SYNC", SI_REF2P, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "NONE",	  SI_REF2N, 0, 0 },
> +	{ "EXT_PPS_OUT",  SI_REF3,  DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "INT_PPS_OUT",  SI_REF4,  DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e823_si_cgu_outputs[] = {
> +	{ "1588-TIME_SYNC", SI_OUT0, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "PHY-CLK",	    SI_OUT1, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> +	{ "10MHZ-SMA2",	    SI_OUT2, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
> +	{ "PPS-SMA1",	    SI_OUT3, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e823_zl_cgu_inputs[] = {
> +	{ "NONE",	  ZL_REF0P, 0, 0 },
> +	{ "INT_PPS_OUT",  ZL_REF0N, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +	{ "SYNCE0_DP",	  ZL_REF1P, DPLL_PIN_TYPE_MUX, 0 },
> +	{ "SYNCE0_DN",	  ZL_REF1N, DPLL_PIN_TYPE_MUX, 0 },
> +	{ "NONE",	  ZL_REF2P, 0, 0 },
> +	{ "NONE",	  ZL_REF2N, 0, 0 },
> +	{ "EXT_CLK_SYNC", ZL_REF3P, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "NONE",	  ZL_REF3N, 0, 0 },
> +	{ "EXT_PPS_OUT",  ZL_REF4P, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +	{ "OCXO",	  ZL_REF4N, DPLL_PIN_TYPE_INT_OSCILLATOR, 0 },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e823_zl_cgu_outputs[] = {
> +	{ "PPS-SMA1",	   ZL_OUT0, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +	{ "10MHZ-SMA2",	   ZL_OUT1, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
> +	{ "PHY-CLK",	   ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> +	{ "1588-TIME_REF", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> +	{ "CPK-TIME_SYNC", ZL_OUT4, DPLL_PIN_TYPE_EXT,
> +		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +	{ "NONE",	   ZL_OUT5, 0, 0 },
> +};
> +
>  extern const struct
>  ice_cgu_pll_params_e822 e822_cgu_params[NUM_ICE_TIME_REF_FREQ];
>  

...