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

Kubalewski, Arkadiusz arkadiusz.kubalewski at intel.com
Fri Jul 28 05:46:39 PDT 2023


>From: Simon Horman <simon.horman at corigine.com>
>Sent: Monday, July 24, 2023 7:22 PM
>
>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.
>

Sure, will fix.

>> + * @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.

Sure, will fix.

>
>> + * @phase_offset: pointer to a buffer for returning phase offset
>> + * @dpll_state: state of the DPLL (output)
>
>And @mode is missing here.
>

Sure, will fix.

>> + *
>> + * 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.
>

No it's leftover, thanks for catching!

>> +		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?
>

Yes, will fix.

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

True, will fix.

>
>...
>
>> 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.

Yes, great catches, will fix.

Thank you for all of them!
Arkadiusz

>
>> +
>> +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];
>>
>
>...



More information about the linux-arm-kernel mailing list