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

[Jacob Keller <jacob.e.keller@intel.com>]

On 8/16/2023 3:54 AM, Przemek Kitszel wrote:
> 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 :)).
> 

There will be users of this function soon which want to get the node
handle out, so I kept that functionality, but I did get rid of the extra
variable. No user should ever care about the node_handle being modified
on error. I'll note it in the function comment though.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan