[Intel-wired-lan] [PATCH net-next v2] ice: Allow 100M speeds for some devices

[Intel-wired-lan] [PATCH net-next v2] ice: Allow 100M speeds for some devices

[Mikael Barsehyan]

From: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>

For certain devices, 100M speeds are supported. Do not mask off
100M speed for these devices.

Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Co-developed-by: Chinh T Cao <chinh.t.cao@intel.com>
Signed-off-by: Chinh T Cao <chinh.t.cao@intel.com>
Signed-off-by: Mikael Barsehyan <mikael.barsehyan@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c  | 20 ++++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h  |  1 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 11 +++++++----
 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 05a4acfbdd1d..010385e67665 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -2775,6 +2775,26 @@ ice_aq_set_port_params(struct ice_port_info *pi, bool double_vlan,
 	return ice_aq_send_cmd(hw, &desc, NULL, 0, cd);
 }
 
+/**
+ * ice_is_100m_speed_supported
+ * @hw: pointer to the HW struct
+ *
+ * returns true if 100M speeds are supported by the device,
+ * false otherwise.
+ */
+bool ice_is_100m_speed_supported(struct ice_hw *hw)
+{
+	switch (hw->device_id) {
+	case ICE_DEV_ID_E822C_SGMII:
+	case ICE_DEV_ID_E822L_SGMII:
+	case ICE_DEV_ID_E823L_1GBE:
+	case ICE_DEV_ID_E823C_SGMII:
+		return true;
+	default:
+		return false;
+	}
+}
+
 /**
  * ice_get_link_speed_based_on_phy_type - returns link speed
  * @phy_type_low: lower part of phy_type
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index a74df1d3a002..2734296bdd3b 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -205,6 +205,7 @@ ice_aq_set_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx, bool value,
 int
 ice_aq_get_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx,
 		bool *value, struct ice_sq_cd *cd);
+bool ice_is_100m_speed_supported(struct ice_hw *hw);
 int
 ice_aq_set_lldp_mib(struct ice_hw *hw, u8 mib_type, void *buf, u16 buf_size,
 		    struct ice_sq_cd *cd);
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 46d8ac7906ea..aa6a0ed8eb97 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -1503,20 +1503,22 @@ ice_get_ethtool_stats(struct net_device *netdev,
 
 /**
  * ice_mask_min_supported_speeds
+ * @hw: pointer to the HW structure
  * @phy_types_high: PHY type high
  * @phy_types_low: PHY type low to apply minimum supported speeds mask
  *
  * Apply minimum supported speeds mask to PHY type low. These are the speeds
  * for ethtool supported link mode.
  */
-static
-void ice_mask_min_supported_speeds(u64 phy_types_high, u64 *phy_types_low)
+static void
+ice_mask_min_supported_speeds(struct ice_hw *hw,
+				   u64 phy_types_high, u64 *phy_types_low)
 {
 	/* if QSFP connection with 100G speed, minimum supported speed is 25G */
 	if (*phy_types_low & ICE_PHY_TYPE_LOW_MASK_100G ||
 	    phy_types_high & ICE_PHY_TYPE_HIGH_MASK_100G)
 		*phy_types_low &= ~ICE_PHY_TYPE_LOW_MASK_MIN_25G;
-	else
+	else if (!ice_is_100m_speed_supported(hw))
 		*phy_types_low &= ~ICE_PHY_TYPE_LOW_MASK_MIN_1G;
 }
 
@@ -1566,7 +1568,8 @@ ice_phy_type_to_ethtool(struct net_device *netdev,
 		phy_types_low = le64_to_cpu(pf->nvm_phy_type_lo);
 		phy_types_high = le64_to_cpu(pf->nvm_phy_type_hi);
 
-		ice_mask_min_supported_speeds(phy_types_high, &phy_types_low);
+		ice_mask_min_supported_speeds(&pf->hw, phy_types_high,
+					      &phy_types_low);
 		/* determine advertised modes based on link override only
 		 * if it's supported and if the FW doesn't abstract the
 		 * driver from having to account for link overrides
-- 
2.35.3

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

Re: [Intel-wired-lan] [PATCH net-next v2] ice: Allow 100M speeds for some devices

[Tony Nguyen]

On 7/28/2022 12:23 PM, Mikael Barsehyan wrote:

<snip>

> @@ -1503,20 +1503,22 @@ ice_get_ethtool_stats(struct net_device *netdev,
>   
>   /**
>    * ice_mask_min_supported_speeds
> + * @hw: pointer to the HW structure
>    * @phy_types_high: PHY type high
>    * @phy_types_low: PHY type low to apply minimum supported speeds mask
>    *
>    * Apply minimum supported speeds mask to PHY type low. These are the speeds
>    * for ethtool supported link mode.
>    */
> -static
> -void ice_mask_min_supported_speeds(u64 phy_types_high, u64 *phy_types_low)
> +static void
> +ice_mask_min_supported_speeds(struct ice_hw *hw,
> +				   u64 phy_types_high, u64 *phy_types_low)

Alignment needs to be adjusted:

Warning - CHECK: Alignment should match open parenthesis

>   {
>   	/* if QSFP connection with 100G speed, minimum supported speed is 25G */
>   	if (*phy_types_low & ICE_PHY_TYPE_LOW_MASK_100G ||
>   	    phy_types_high & ICE_PHY_TYPE_HIGH_MASK_100G)
>   		*phy_types_low &= ~ICE_PHY_TYPE_LOW_MASK_MIN_25G;
> -	else
> +	else if (!ice_is_100m_speed_supported(hw))
>   		*phy_types_low &= ~ICE_PHY_TYPE_LOW_MASK_MIN_1G;
>   }
>   
> @@ -1566,7 +1568,8 @@ ice_phy_type_to_ethtool(struct net_device *netdev,
>   		phy_types_low = le64_to_cpu(pf->nvm_phy_type_lo);
>   		phy_types_high = le64_to_cpu(pf->nvm_phy_type_hi);
>   
> -		ice_mask_min_supported_speeds(phy_types_high, &phy_types_low);
> +		ice_mask_min_supported_speeds(&pf->hw, phy_types_high,
> +					      &phy_types_low);
>   		/* determine advertised modes based on link override only
>   		 * if it's supported and if the FW doesn't abstract the
>   		 * driver from having to account for link overrides
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v2] ice: Allow 100M speeds for some devices

[Paul Menzel]

Dear Mikael, dear Chinh,


Thank you for your patch.

Am 28.07.22 um 21:23 schrieb Mikael Barsehyan:
> From: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> 
> For certain devices, 100M speeds are supported. Do not mask off
> 100M speed for these devices.

Please list the devices in the commit message.

Please also describe the implementation in the commit message.

How did you test this?

> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> Co-developed-by: Chinh T Cao <chinh.t.cao@intel.com>
> Signed-off-by: Chinh T Cao <chinh.t.cao@intel.com>
> Signed-off-by: Mikael Barsehyan <mikael.barsehyan@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_common.c  | 20 ++++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_common.h  |  1 +
>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 11 +++++++----
>   3 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 05a4acfbdd1d..010385e67665 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -2775,6 +2775,26 @@ ice_aq_set_port_params(struct ice_port_info *pi, bool double_vlan,
>   	return ice_aq_send_cmd(hw, &desc, NULL, 0, cd);
>   }
>   
> +/**
> + * ice_is_100m_speed_supported
> + * @hw: pointer to the HW struct
> + *
> + * returns true if 100M speeds are supported by the device,
> + * false otherwise.
> + */
> +bool ice_is_100m_speed_supported(struct ice_hw *hw)
> +{
> +	switch (hw->device_id) {
> +	case ICE_DEV_ID_E822C_SGMII:
> +	case ICE_DEV_ID_E822L_SGMII:
> +	case ICE_DEV_ID_E823L_1GBE:
> +	case ICE_DEV_ID_E823C_SGMII:
> +		return true;
> +	default:
> +		return false;
> +	}

Is there no way to determine this during run-time without maintaining a 
list?

> +}
> +
>   /**
>    * ice_get_link_speed_based_on_phy_type - returns link speed
>    * @phy_type_low: lower part of phy_type
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
> index a74df1d3a002..2734296bdd3b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.h
> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
> @@ -205,6 +205,7 @@ ice_aq_set_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx, bool value,
>   int
>   ice_aq_get_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx,
>   		bool *value, struct ice_sq_cd *cd);
> +bool ice_is_100m_speed_supported(struct ice_hw *hw);

I’d name it `is_100mbits_supported`.

>   int
>   ice_aq_set_lldp_mib(struct ice_hw *hw, u8 mib_type, void *buf, u16 buf_size,
>   		    struct ice_sq_cd *cd);
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index 46d8ac7906ea..aa6a0ed8eb97 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -1503,20 +1503,22 @@ ice_get_ethtool_stats(struct net_device *netdev,
>   
>   /**
>    * ice_mask_min_supported_speeds
> + * @hw: pointer to the HW structure
>    * @phy_types_high: PHY type high
>    * @phy_types_low: PHY type low to apply minimum supported speeds mask
>    *
>    * Apply minimum supported speeds mask to PHY type low. These are the speeds
>    * for ethtool supported link mode.
>    */
> -static
> -void ice_mask_min_supported_speeds(u64 phy_types_high, u64 *phy_types_low)
> +static void
> +ice_mask_min_supported_speeds(struct ice_hw *hw,
> +				   u64 phy_types_high, u64 *phy_types_low)
>   {
>   	/* if QSFP connection with 100G speed, minimum supported speed is 25G */
>   	if (*phy_types_low & ICE_PHY_TYPE_LOW_MASK_100G ||
>   	    phy_types_high & ICE_PHY_TYPE_HIGH_MASK_100G)
>   		*phy_types_low &= ~ICE_PHY_TYPE_LOW_MASK_MIN_25G;
> -	else
> +	else if (!ice_is_100m_speed_supported(hw))
>   		*phy_types_low &= ~ICE_PHY_TYPE_LOW_MASK_MIN_1G;
>   }
>   
> @@ -1566,7 +1568,8 @@ ice_phy_type_to_ethtool(struct net_device *netdev,
>   		phy_types_low = le64_to_cpu(pf->nvm_phy_type_lo);
>   		phy_types_high = le64_to_cpu(pf->nvm_phy_type_hi);
>   
> -		ice_mask_min_supported_speeds(phy_types_high, &phy_types_low);
> +		ice_mask_min_supported_speeds(&pf->hw, phy_types_high,
> +					      &phy_types_low);
>   		/* determine advertised modes based on link override only
>   		 * if it's supported and if the FW doesn't abstract the
>   		 * driver from having to account for link overrides


Kind regards,

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

Re: [Intel-wired-lan] [PATCH net-next v2] ice: Allow 100M speeds for some devices

[Anirudh Venkataramanan]

> Am 28.07.22 um 21:23 schrieb Mikael Barsehyan:
>> From: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
>>
>> For certain devices, 100M speeds are supported. Do not mask off
>> 100M speed for these devices.
> 
> Please list the devices in the commit message.

So you're asking that device IDs be listed in the commit message? Seems 
a bit redundant seeing as they can be inferred from the code quite 
easily. Let me know if you feel otherwise.

> 
> Please also describe the implementation in the commit message.

My understanding is that commit messages should list the what and the 
why, not the how.

> 
> How did you test this?

For ethtool <ethX>, 100M will not be listed as a link mode for devices 
that don't support it.

> 
>> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
>> Co-developed-by: Chinh T Cao <chinh.t.cao@intel.com>
>> Signed-off-by: Chinh T Cao <chinh.t.cao@intel.com>
>> Signed-off-by: Mikael Barsehyan <mikael.barsehyan@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_common.c  | 20 ++++++++++++++++++++
>>   drivers/net/ethernet/intel/ice/ice_common.h  |  1 +
>>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 11 +++++++----
>>   3 files changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c 
>> b/drivers/net/ethernet/intel/ice/ice_common.c
>> index 05a4acfbdd1d..010385e67665 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>> @@ -2775,6 +2775,26 @@ ice_aq_set_port_params(struct ice_port_info 
>> *pi, bool double_vlan,
>>       return ice_aq_send_cmd(hw, &desc, NULL, 0, cd);
>>   }
>> +/**
>> + * ice_is_100m_speed_supported
>> + * @hw: pointer to the HW struct
>> + *
>> + * returns true if 100M speeds are supported by the device,
>> + * false otherwise.
>> + */
>> +bool ice_is_100m_speed_supported(struct ice_hw *hw)
>> +{
>> +    switch (hw->device_id) {
>> +    case ICE_DEV_ID_E822C_SGMII:
>> +    case ICE_DEV_ID_E822L_SGMII:
>> +    case ICE_DEV_ID_E823L_1GBE:
>> +    case ICE_DEV_ID_E823C_SGMII:
>> +        return true;
>> +    default:
>> +        return false;
>> +    }
> 
> Is there no way to determine this during run-time without maintaining a 
> list?

No.

> 
>> +}
>> +
>>   /**
>>    * ice_get_link_speed_based_on_phy_type - returns link speed
>>    * @phy_type_low: lower part of phy_type
>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h 
>> b/drivers/net/ethernet/intel/ice/ice_common.h
>> index a74df1d3a002..2734296bdd3b 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_common.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
>> @@ -205,6 +205,7 @@ ice_aq_set_gpio(struct ice_hw *hw, u16 
>> gpio_ctrl_handle, u8 pin_idx, bool value,
>>   int
>>   ice_aq_get_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx,
>>           bool *value, struct ice_sq_cd *cd);
>> +bool ice_is_100m_speed_supported(struct ice_hw *hw);
> 
> I’d name it `is_100mbits_supported`.

Naming is a bit subjective I suppose. As long as the function name is 
sensible and readable, it's fine.

To each their own I suppose. :-)

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

Re: [Intel-wired-lan] [PATCH net-next v2] ice: Allow 100M speeds for some devices

[Rustad, Mark D]

[-- Attachment #1.1: Type: text/plain, Size: 1068 bytes --]

> On Aug 8, 2022, at 10:44 AM, Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> wrote:
> 
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
>>> index a74df1d3a002..2734296bdd3b 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_common.h
>>> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
>>> @@ -205,6 +205,7 @@ ice_aq_set_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx, bool value,
>>> int
>>> ice_aq_get_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx,
>>> bool *value, struct ice_sq_cd *cd);
>>> +bool ice_is_100m_speed_supported(struct ice_hw *hw);
>> I’d name it `is_100mbits_supported`.
> 
> Naming is a bit subjective I suppose. As long as the function name is sensible and readable, it's fine.
> 
> To each their own I suppose. :-)

It really is better to always have the driver prefix on symbols to avoid any possible namespace clashes. So, it is best as it was in this case.

--
Mark Rustad (he/him), Ethernet Products Group, Intel Corporation


[-- Attachment #1.2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

[-- Attachment #2: Type: text/plain, Size: 162 bytes --]

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

Re: [Intel-wired-lan] [PATCH net-next v2] ice: Allow 100M speeds for some devices

[Paul Menzel]

Dear Mark,


Am 11.08.22 um 02:10 schrieb Rustad, Mark D:
>> On Aug 8, 2022, at 10:44 AM, Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> wrote:
>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
>>>> index a74df1d3a002..2734296bdd3b 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_common.h
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
>>>> @@ -205,6 +205,7 @@ ice_aq_set_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx, bool value,
>>>> int
>>>> ice_aq_get_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx,
>>>> bool *value, struct ice_sq_cd *cd);
>>>> +bool ice_is_100m_speed_supported(struct ice_hw *hw);
>>> I’d name it `is_100mbits_supported`.
>>
>> Naming is a bit subjective I suppose. As long as the function name is sensible and readable, it's fine.
>>
>> To each their own I suppose. :-)
> 
> It really is better to always have the driver prefix on symbols to avoid any possible namespace clashes. So, it is best as it was in this case.

My comment was about the unit in the name, and not the prefix.


Kind regards,

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