Message-ID: <SJ0PR11MB58667D4EC93EE07538F4C3AEE5BB2@SJ0PR11MB5866.namprd11.prod.outlook.com>
Date: Tue, 22 Apr 2025 17:03:16 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Kolacinski, Karol"
	<karol.kolacinski@...el.com>, "Olech, Milena" <milena.olech@...el.com>,
	"Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH iwl-next v5 2/3] ice: change SMA pins to
 SDP in PTP API



> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of
> Arkadiusz Kubalewski
> Sent: Tuesday, April 22, 2025 6:02 PM
> To: intel-wired-lan@...ts.osuosl.org
> Cc: netdev@...r.kernel.org; Kolacinski, Karol <karol.kolacinski@...el.com>;
> Olech, Milena <milena.olech@...el.com>; Kubalewski, Arkadiusz
> <arkadiusz.kubalewski@...el.com>
> Subject: [Intel-wired-lan] [PATCH iwl-next v5 2/3] ice: change SMA pins to SDP
> in PTP API
> 
> From: Karol Kolacinski <karol.kolacinski@...el.com>
> 
> This change aligns E810 PTP pin control to all other products.
> 
> Currently, SMA/U.FL port expanders are controlled together with SDP pins
> connected to 1588 clock. To align this, separate this control by exposing only
> SDP20..23 pins in PTP API on adapters with DPLL.
> 
> Clear error for all E810 on absent NVM pin section or other errors to allow
> proper initialization on SMA E810 with NVM section.
> 
> Use ARRAY_SIZE for pin array instead of internal definition.
> 
> Reviewed-by: Milena Olech <milena.olech@...el.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@...el.com>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
> ---
> v5:
> - no change.
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 254 ++++-------------------
>  drivers/net/ethernet/intel/ice/ice_ptp.h |   3 -
>  2 files changed, 39 insertions(+), 218 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index b79a148ed0f2..b948a6d9226c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -40,21 +40,19 @@ static const struct ice_ptp_pin_desc
> ice_pin_desc_e810[] = {
>  	{  ONE_PPS,   { -1,  5 }, { 0, 1 }},
>  };
> 
> -static const char ice_pin_names_nvm[][64] = {
> -	"GNSS",
> -	"SMA1",
> -	"U.FL1",
> -	"SMA2",
> -	"U.FL2",
> +static const char ice_pin_names_dpll[][64] = {
> +	"SDP20",
> +	"SDP21",
> +	"SDP22",
> +	"SDP23",
>  };
> 
> -static const struct ice_ptp_pin_desc ice_pin_desc_e810_sma[] = {
> +static const struct ice_ptp_pin_desc ice_pin_desc_dpll[] = {
>  	/* name,   gpio,       delay */
> -	{  GNSS, {  1, -1 }, { 0, 0 }},
> -	{  SMA1, {  1,  0 }, { 0, 1 }},
> -	{  UFL1, { -1,  0 }, { 0, 1 }},
> -	{  SMA2, {  3,  2 }, { 0, 1 }},
> -	{  UFL2, {  3, -1 }, { 0, 0 }},
> +	{  SDP0, { -1,  0 }, { 0, 1 }},
> +	{  SDP1, {  1, -1 }, { 0, 0 }},
> +	{  SDP2, { -1,  2 }, { 0, 1 }},
> +	{  SDP3, {  3, -1 }, { 0, 0 }},
>  };
> 
>  static struct ice_pf *ice_get_ctrl_pf(struct ice_pf *pf) @@ -92,101 +90,6 @@
> static int ice_ptp_find_pin_idx(struct ice_pf *pf, enum ptp_pin_function func,
>  	return -1;
>  }
> 
> -/**
> - * ice_ptp_update_sma_data - update SMA pins data according to pins setup
> - * @pf: Board private structure
> - * @sma_pins: parsed SMA pins status
> - * @data: SMA data to update
> - */
> -static void ice_ptp_update_sma_data(struct ice_pf *pf, unsigned int
> sma_pins[],
> -				    u8 *data)
> -{
> -	const char *state1, *state2;
> -
> -	/* Set the right state based on the desired configuration.
> -	 * When bit is set, functionality is disabled.
> -	 */
> -	*data &= ~ICE_ALL_SMA_MASK;
> -	if (!sma_pins[UFL1 - 1]) {
> -		if (sma_pins[SMA1 - 1] == PTP_PF_EXTTS) {
> -			state1 = "SMA1 Rx, U.FL1 disabled";
> -			*data |= ICE_SMA1_TX_EN;
> -		} else if (sma_pins[SMA1 - 1] == PTP_PF_PEROUT) {
> -			state1 = "SMA1 Tx U.FL1 disabled";
> -			*data |= ICE_SMA1_DIR_EN;
> -		} else {
> -			state1 = "SMA1 disabled, U.FL1 disabled";
> -			*data |= ICE_SMA1_MASK;
> -		}
> -	} else {
> -		/* U.FL1 Tx will always enable SMA1 Rx */
> -		state1 = "SMA1 Rx, U.FL1 Tx";
> -	}
> -
> -	if (!sma_pins[UFL2 - 1]) {
> -		if (sma_pins[SMA2 - 1] == PTP_PF_EXTTS) {
> -			state2 = "SMA2 Rx, U.FL2 disabled";
> -			*data |= ICE_SMA2_TX_EN |
> ICE_SMA2_UFL2_RX_DIS;
> -		} else if (sma_pins[SMA2 - 1] == PTP_PF_PEROUT) {
> -			state2 = "SMA2 Tx, U.FL2 disabled";
> -			*data |= ICE_SMA2_DIR_EN |
> ICE_SMA2_UFL2_RX_DIS;
> -		} else {
> -			state2 = "SMA2 disabled, U.FL2 disabled";
> -			*data |= ICE_SMA2_MASK;
> -		}
> -	} else {
> -		if (!sma_pins[SMA2 - 1]) {
> -			state2 = "SMA2 disabled, U.FL2 Rx";
> -			*data |= ICE_SMA2_DIR_EN | ICE_SMA2_TX_EN;
> -		} else {
> -			state2 = "SMA2 Tx, U.FL2 Rx";
> -			*data |= ICE_SMA2_DIR_EN;
> -		}
> -	}
> -
> -	dev_dbg(ice_pf_to_dev(pf), "%s, %s\n", state1, state2);
> -}
> -
> -/**
> - * ice_ptp_set_sma_cfg - set the configuration of the SMA control logic
> - * @pf: Board private structure
> - *
> - * Return: 0 on success, negative error code otherwise
> - */
> -static int ice_ptp_set_sma_cfg(struct ice_pf *pf) -{
> -	const struct ice_ptp_pin_desc *ice_pins = pf->ptp.ice_pin_desc;
> -	struct ptp_pin_desc *pins = pf->ptp.pin_desc;
> -	unsigned int sma_pins[ICE_SMA_PINS_NUM] = {};
> -	int err;
> -	u8 data;
> -
> -	/* Read initial pin state value */
> -	err = ice_read_sma_ctrl(&pf->hw, &data);
> -	if (err)
> -		return err;
> -
> -	/* Get SMA/U.FL pins states */
> -	for (int i = 0; i < pf->ptp.info.n_pins; i++)
> -		if (pins[i].func) {
> -			int name_idx = ice_pins[i].name_idx;
> -
> -			switch (name_idx) {
> -			case SMA1:
> -			case UFL1:
> -			case SMA2:
> -			case UFL2:
> -				sma_pins[name_idx - 1] = pins[i].func;
> -				break;
> -			default:
> -				continue;
> -			}
> -		}
> -
> -	ice_ptp_update_sma_data(pf, sma_pins, &data);
> -	return ice_write_sma_ctrl(&pf->hw, data);
> -}
> -
>  /**
>   * ice_ptp_cfg_tx_interrupt - Configure Tx timestamp interrupt for the device
>   * @pf: Board private structure
> @@ -1878,63 +1781,6 @@ static void ice_ptp_enable_all_perout(struct
> ice_pf *pf)
>  					   true);
>  }
> 
> -/**
> - * ice_ptp_disable_shared_pin - Disable enabled pin that shares GPIO
> - * @pf: Board private structure
> - * @pin: Pin index
> - * @func: Assigned function
> - *
> - * Return: 0 on success, negative error code otherwise
> - */
> -static int ice_ptp_disable_shared_pin(struct ice_pf *pf, unsigned int pin,
> -				      enum ptp_pin_function func)
> -{
> -	unsigned int gpio_pin;
> -
> -	switch (func) {
> -	case PTP_PF_PEROUT:
> -		gpio_pin = pf->ptp.ice_pin_desc[pin].gpio[1];
> -		break;
> -	case PTP_PF_EXTTS:
> -		gpio_pin = pf->ptp.ice_pin_desc[pin].gpio[0];
> -		break;
> -	default:
> -		return -EOPNOTSUPP;
> -	}
> -
> -	for (unsigned int i = 0; i < pf->ptp.info.n_pins; i++) {
> -		struct ptp_pin_desc *pin_desc = &pf->ptp.pin_desc[i];
> -		unsigned int chan = pin_desc->chan;
> -
> -		/* Skip pin idx from the request */
> -		if (i == pin)
> -			continue;
> -
> -		if (pin_desc->func == PTP_PF_PEROUT &&
> -		    pf->ptp.ice_pin_desc[i].gpio[1] == gpio_pin) {
> -			pf->ptp.perout_rqs[chan].period.sec = 0;
> -			pf->ptp.perout_rqs[chan].period.nsec = 0;
> -			pin_desc->func = PTP_PF_NONE;
> -			pin_desc->chan = 0;
> -			dev_dbg(ice_pf_to_dev(pf), "Disabling pin %u with
> shared output GPIO pin %u\n",
> -				i, gpio_pin);
> -			return ice_ptp_cfg_perout(pf, &pf-
> >ptp.perout_rqs[chan],
> -						  false);
> -		} else if (pf->ptp.pin_desc->func == PTP_PF_EXTTS &&
> -			   pf->ptp.ice_pin_desc[i].gpio[0] == gpio_pin) {
> -			pf->ptp.extts_rqs[chan].flags &=
> ~PTP_ENABLE_FEATURE;
> -			pin_desc->func = PTP_PF_NONE;
> -			pin_desc->chan = 0;
> -			dev_dbg(ice_pf_to_dev(pf), "Disabling pin %u with
> shared input GPIO pin %u\n",
> -				i, gpio_pin);
> -			return ice_ptp_cfg_extts(pf, &pf-
> >ptp.extts_rqs[chan],
> -						 false);
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  /**
>   * ice_verify_pin - verify if pin supports requested pin function
>   * @info: the driver's PTP info structure @@ -1969,14 +1815,6 @@ static int
> ice_verify_pin(struct ptp_clock_info *info, unsigned int pin,
>  		return -EOPNOTSUPP;
>  	}
> 
> -	/* On adapters with SMA_CTRL disable other pins that share same
> GPIO */
> -	if (ice_is_feature_supported(pf, ICE_F_SMA_CTRL)) {
> -		ice_ptp_disable_shared_pin(pf, pin, func);
> -		pf->ptp.pin_desc[pin].func = func;
> -		pf->ptp.pin_desc[pin].chan = chan;
> -		return ice_ptp_set_sma_cfg(pf);
> -	}
> -
>  	return 0;
>  }
> 
> @@ -2499,14 +2337,14 @@ static void ice_ptp_setup_pin_cfg(struct ice_pf
> *pf)
>  	for (unsigned int i = 0; i < pf->ptp.info.n_pins; i++) {
>  		const struct ice_ptp_pin_desc *desc = &pf-
> >ptp.ice_pin_desc[i];
>  		struct ptp_pin_desc *pin = &pf->ptp.pin_desc[i];
> -		const char *name = NULL;
> +		const char *name;
> 
>  		if (!ice_is_feature_supported(pf, ICE_F_SMA_CTRL))
>  			name = ice_pin_names[desc->name_idx];
> -		else if (desc->name_idx != GPIO_NA)
> -			name = ice_pin_names_nvm[desc->name_idx];
> -		if (name)
> -			strscpy(pin->name, name, sizeof(pin->name));
> +		else
> +			name = ice_pin_names_dpll[desc->name_idx];
> +
> +		strscpy(pin->name, name, sizeof(pin->name));
> 
>  		pin->index = i;
>  	}
> @@ -2518,8 +2356,8 @@ static void ice_ptp_setup_pin_cfg(struct ice_pf *pf)
>   * ice_ptp_disable_pins - Disable PTP pins
>   * @pf: pointer to the PF structure
>   *
> - * Disable the OS access to the SMA pins. Called to clear out the OS
> - * indications of pin support when we fail to setup the SMA control register.
> + * Disable the OS access to the pins. Called to clear out the OS
> + * indications of pin support when we fail to setup pin array.
>   */
>  static void ice_ptp_disable_pins(struct ice_pf *pf)  { @@ -2560,40 +2398,30
> @@ static int ice_ptp_parse_sdp_entries(struct ice_pf *pf, __le16 *entries,
>  	for (i = 0; i < num_entries; i++) {
>  		u16 entry = le16_to_cpu(entries[i]);
>  		DECLARE_BITMAP(bitmap, GPIO_NA);
> -		unsigned int bitmap_idx;
> +		unsigned int idx;
>  		bool dir;
>  		u16 gpio;
> 
>  		*bitmap = FIELD_GET(ICE_AQC_NVM_SDP_AC_PIN_M,
> entry);
> +
> +		/* Check if entry's pin bitmap is valid. */
> +		if (bitmap_empty(bitmap, GPIO_NA))
> +			continue;
> +
>  		dir = !!FIELD_GET(ICE_AQC_NVM_SDP_AC_DIR_M, entry);
>  		gpio = FIELD_GET(ICE_AQC_NVM_SDP_AC_SDP_NUM_M,
> entry);
> -		for_each_set_bit(bitmap_idx, bitmap, GPIO_NA + 1) {
> -			unsigned int idx;
> 
> -			/* Check if entry's pin bit is valid */
> -			if (bitmap_idx >= NUM_PTP_PINS_NVM &&
> -			    bitmap_idx != GPIO_NA)
> -				continue;
> -
> -			/* Check if pin already exists */
> -			for (idx = 0; idx < ICE_N_PINS_MAX; idx++)
> -				if (pins[idx].name_idx == bitmap_idx)
> -					break;
> -
> -			if (idx == ICE_N_PINS_MAX) {
> -				/* Pin not found, setup its entry and name */
> -				idx = n_pins++;
> -				pins[idx].name_idx = bitmap_idx;
> -				if (bitmap_idx == GPIO_NA)
> -					strscpy(pf->ptp.pin_desc[idx].name,
> -						ice_pin_names[gpio],
> -						sizeof(pf->ptp.pin_desc[idx]
> -							       .name));
> -			}
> +		for (idx = 0; idx < ICE_N_PINS_MAX; idx++) {
> +			if (pins[idx].name_idx == gpio)
> +				break;
> +		}
> 
> -			/* Setup in/out GPIO number */
> -			pins[idx].gpio[dir] = gpio;
> +		if (idx == ICE_N_PINS_MAX) {
> +			/* Pin not found, setup its entry and name */
> +			idx = n_pins++;
> +			pins[idx].name_idx = gpio;
>  		}
> +		pins[idx].gpio[dir] = gpio;
>  	}
> 
>  	for (i = 0; i < n_pins; i++) {
> @@ -2621,10 +2449,10 @@ static void ice_ptp_set_funcs_e82x(struct ice_pf
> *pf)
> 
>  	if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
>  		pf->ptp.ice_pin_desc = ice_pin_desc_e825c;
> -		pf->ptp.info.n_pins =
> ICE_PIN_DESC_ARR_LEN(ice_pin_desc_e825c);
> +		pf->ptp.info.n_pins = ARRAY_SIZE(ice_pin_desc_e825c);
>  	} else {
>  		pf->ptp.ice_pin_desc = ice_pin_desc_e82x;
> -		pf->ptp.info.n_pins =
> ICE_PIN_DESC_ARR_LEN(ice_pin_desc_e82x);
> +		pf->ptp.info.n_pins = ARRAY_SIZE(ice_pin_desc_e82x);
>  	}
>  	ice_ptp_setup_pin_cfg(pf);
>  }
> @@ -2650,15 +2478,13 @@ static void ice_ptp_set_funcs_e810(struct ice_pf
> *pf)
>  	if (err) {
>  		/* SDP section does not exist in NVM or is corrupted */
>  		if (ice_is_feature_supported(pf, ICE_F_SMA_CTRL)) {
> -			ptp->ice_pin_desc = ice_pin_desc_e810_sma;
> -			ptp->info.n_pins =
> -
> 	ICE_PIN_DESC_ARR_LEN(ice_pin_desc_e810_sma);
> +			ptp->ice_pin_desc = ice_pin_desc_dpll;
> +			ptp->info.n_pins = ARRAY_SIZE(ice_pin_desc_dpll);
>  		} else {
>  			pf->ptp.ice_pin_desc = ice_pin_desc_e810;
> -			pf->ptp.info.n_pins =
> -				ICE_PIN_DESC_ARR_LEN(ice_pin_desc_e810);
> -			err = 0;
> +			pf->ptp.info.n_pins =
> ARRAY_SIZE(ice_pin_desc_e810);
>  		}
> +		err = 0;
>  	} else {
>  		desc = devm_kcalloc(ice_pf_to_dev(pf), ICE_N_PINS_MAX,
>  				    sizeof(struct ice_ptp_pin_desc), @@ -
> 2676,8 +2502,6 @@ static void ice_ptp_set_funcs_e810(struct ice_pf *pf)
>  	ptp->info.pin_config = ptp->pin_desc;
>  	ice_ptp_setup_pin_cfg(pf);
> 
> -	if (ice_is_feature_supported(pf, ICE_F_SMA_CTRL))
> -		err = ice_ptp_set_sma_cfg(pf);
>  err:
>  	if (err) {
>  		devm_kfree(ice_pf_to_dev(pf), desc);
> @@ -2703,7 +2527,7 @@ static void ice_ptp_set_funcs_e830(struct ice_pf
> *pf)  #endif /* CONFIG_ICE_HWTS */
>  	/* Rest of the config is the same as base E810 */
>  	pf->ptp.ice_pin_desc = ice_pin_desc_e810;
> -	pf->ptp.info.n_pins = ICE_PIN_DESC_ARR_LEN(ice_pin_desc_e810);
> +	pf->ptp.info.n_pins = ARRAY_SIZE(ice_pin_desc_e810);
>  	ice_ptp_setup_pin_cfg(pf);
>  }
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h
> b/drivers/net/ethernet/intel/ice/ice_ptp.h
> index 3b769a0cad00..c8dac5a5bcd9 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
> @@ -202,9 +202,6 @@ enum ice_ptp_pin_nvm {
> 
>  /* Pin definitions for PTP */
>  #define ICE_N_PINS_MAX			6
> -#define ICE_SMA_PINS_NUM		4
> -#define ICE_PIN_DESC_ARR_LEN(_arr)	(sizeof(_arr) / \
> -					 sizeof(struct ice_ptp_pin_desc))
> 
>  /**
>   * struct ice_ptp_pin_desc - hardware pin description data
> --
> 2.38.1

Powered by blists - more mailing lists