Re: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before enabling ICE_F_SMA_CTRL

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 8/16/23 00:35, Jacob Keller wrote:
> Currently, the ice driver unconditionally enables ICE_F_SMA_CTRL for all
> E810-T based devices. In some cases, the SMA control access is not
> available in the netlist firmware component. In such a case, the driver
> will fail to setup the SMA pins. When this happens, the driver prints
> "Failed to configure E810-T SMA pin control" and forcibly disables all PTP
> pin configuration support.
> 
> This results in failure to use even the fixed pin capabilities of standard
> E810 devices, resulting in reduced functionality.
> 
> To avoid this, check the netlist for the SMA control module before enabling
> the ICE_F_SMA_CTRL feature. If the netlist lacks this module, then the
> feature will not be enabled. In this case, the driver flow for enabling
> periodic outputs and external timestamps will fall back to the standard
> fixed pin configuration.
> 
> This allows supporting the software defined pins on a wider array of
> platforms.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Overall it's a nice series!

> ---
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  6 ++-
>   drivers/net/ethernet/intel/ice/ice_common.c   | 46 +++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_common.h   |  3 ++
>   drivers/net/ethernet/intel/ice/ice_lib.c      |  3 +-
>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 16 +++++++
>   drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  1 +
>   6 files changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index 90616750e779..82c4daf0a825 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -1367,6 +1367,7 @@ struct ice_aqc_link_topo_params {
>   #define ICE_AQC_LINK_TOPO_NODE_TYPE_CAGE	6
>   #define ICE_AQC_LINK_TOPO_NODE_TYPE_MEZZ	7
>   #define ICE_AQC_LINK_TOPO_NODE_TYPE_ID_EEPROM	8
> +#define ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_MUX	10
>   #define ICE_AQC_LINK_TOPO_NODE_CTX_S		4
>   #define ICE_AQC_LINK_TOPO_NODE_CTX_M		\
>   				(0xF << ICE_AQC_LINK_TOPO_NODE_CTX_S)
> @@ -1403,8 +1404,9 @@ struct ice_aqc_link_topo_addr {
>   struct ice_aqc_get_link_topo {
>   	struct ice_aqc_link_topo_addr addr;
>   	u8 node_part_num;
> -#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575	0x21
> -#define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827	0x31
> +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575			0x21
> +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827			0x31
> +#define ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX		0x47
>   	u8 rsvd[9];
>   };
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 2652e4f5c4a2..9eeda3f5aa75 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -2503,6 +2503,52 @@ bool ice_is_pf_c827(struct ice_hw *hw)
>   	return false;
>   }
>   
> +#define MAX_NETLIST_SIZE 10
> +/**
> + * ice_find_netlist_node
> + * @hw: pointer to the hw struct
> + * @node_type_ctx: type of netlist node to look for
> + * @node_part_number: node part number to look for
> + * @node_handle: output parameter if node found - optional
> + *
> + * Find and return the node handle for a given node type and part number in the
> + * netlist. When found 0 is returned, -ENOENT otherwise. If
> + * node_handle provided, it would be set to found node handle.
> + */
> +int
> +ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number,
> +		      u16 *node_handle)
> +{
> +	struct ice_aqc_get_link_topo cmd;
> +	u8 rec_node_part_number;
> +	u16 rec_node_handle;

I see that you are using separate variable to 'do not touch' 
@node_handle param if it does not need to be updated.
But perhaps you could consider to just pass @node_handle in place of 
@rec_node_handle below, and have less code?
I do not see any non-null usage of the field anyway.

(rationale: our code is so self-similar that I needed to check wheater 
you are basing-of recent changes by Jan&Karol or re-doing them ;)
answer: we are fine here :)).

> +	u8 idx;
> +
> +	for (idx = 0; idx < MAX_NETLIST_SIZE; idx++) {
> +		int status;
> +
> +		memset(&cmd, 0, sizeof(cmd));
> +
> +		cmd.addr.topo_params.node_type_ctx =
> +			(node_type_ctx << ICE_AQC_LINK_TOPO_NODE_TYPE_S);

I would FIELD_PREP() here

and perhaps convert @cmd scope to inside the loop, that would enable = { 
.addr.topo_params = { ... } } declaration

> +		cmd.addr.topo_params.index = idx;
> +
> +		status = ice_aq_get_netlist_node(hw, &cmd,
> +						 &rec_node_part_number,
> +						 &rec_node_handle);
> +		if (status)
> +			return status;
> +
> +		if (rec_node_part_number == node_part_number) {
> +			if (node_handle)
> +				*node_handle = rec_node_handle;
> +			return 0;
> +		}
> +	}
> +
> +	return -ENOENT;
> +}
> +
>   /**
>    * ice_aq_list_caps - query function/device capabilities
>    * @hw: pointer to the HW struct
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
> index 2250a9c5f61e..f7178a5686a5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.h
> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
> @@ -94,6 +94,9 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool qual_mods, u8 report_mode,
>   		    struct ice_sq_cd *cd);
>   bool ice_is_pf_c827(struct ice_hw *hw);
>   int
> +ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number,
> +		      u16 *node_handle);
> +int
>   ice_aq_list_caps(struct ice_hw *hw, void *buf, u16 buf_size, u32 *cap_count,
>   		 enum ice_adminq_opc opc, struct ice_sq_cd *cd);
>   int
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index f29ff54383b5..4ac8998cb964 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -3989,8 +3989,9 @@ void ice_init_feature_support(struct ice_pf *pf)
>   		/* If we don't own the timer - don't enable other caps */
>   		if (!ice_pf_src_tmr_owned(pf))
>   			break;
> -		if (ice_is_e810t(&pf->hw)) {
> +		if (ice_is_clock_mux_present_e810t(&pf->hw))
>   			ice_set_feature_support(pf, ICE_F_SMA_CTRL);
> +		if (ice_is_e810t(&pf->hw)) {
>   			if (ice_gnss_is_gps_present(&pf->hw))
>   				ice_set_feature_support(pf, ICE_F_GNSS);
>   		}
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index fd19afaf9c85..bd3f32bfbc78 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -3018,6 +3018,22 @@ ice_get_pca9575_handle(struct ice_hw *hw, u16 *pca9575_handle)
>   	return 0;
>   }
>   
> +/**
> + * ice_is_clock_mux_present_e810t
> + * @hw: pointer to the hw struct
> + *
> + * Check if the Clock Multiplexer device is present in the netlist
> + */
> +bool ice_is_clock_mux_present_e810t(struct ice_hw *hw)
> +{
> +	if (ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_MUX,
> +				  ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX,
> +				  NULL))
> +		return false;
> +
> +	return true;
> +}
> +
>   /**
>    * ice_read_sma_ctrl_e810t
>    * @hw: pointer to the hw struct
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> index 4e3c1382c477..3768e7a01920 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> @@ -195,6 +195,7 @@ int ice_phy_cfg_tx_offset_e822(struct ice_hw *hw, u8 port);
>   int ice_phy_cfg_rx_offset_e822(struct ice_hw *hw, u8 port);
>   
>   /* E810 family functions */
> +bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
>   int ice_ptp_init_phy_e810(struct ice_hw *hw);
>   int ice_read_sma_ctrl_e810t(struct ice_hw *hw, u8 *data);
>   int ice_write_sma_ctrl_e810t(struct ice_hw *hw, u8 data);

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan