[PATCH] igb: conditionalize I2C bit banging on external thermal sensor support

[PATCH] igb: conditionalize I2C bit banging on external thermal sensor support

[Corinna Vinschen]

Commit a97f8783a937 ("igb: unbreak I2C bit-banging on i350") introduced
code to change I2C settings to bit banging unconditionally.

However, this patch introduced a regression:  On an Intel S2600CWR
Server Board with three NICs:

- 1x dual-port copper
  Intel I350 Gigabit Network Connection [8086:1521] (rev 01)
  fw 1.63, 0x80000dda

- 2x quad-port SFP+ with copper SFP Avago ABCU-5700RZ
  Intel I350 Gigabit Fiber Network Connection [8086:1522] (rev 01)
  fw 1.52.0

the SFP NICs no longer get link at all.  Reverting commit a97f8783a937
or switching to the Intel out-of-tree driver both fix the problem.

Per the igb out-of-tree driver, I2C bit banging on i350 depends on
support for an external thermal sensor (ETS).  However, commit
a97f8783a937 added bit banging unconditionally.  Additionally, the
out-of-tree driver always calls init_thermal_sensor_thresh on probe,
while our driver only calls init_thermal_sensor_thresh only in
igb_reset(), and only if an ETS is present, ignoring the internal
thermal sensor.  The affected SFPs don't provide an ETS.  Per Intel,
the behaviour is a result of i350 firmware requirements.

This patch fixes the problem by aligning the behaviour to the
out-of-tree driver:

- split igb_init_i2c() into two functions:
  - igb_init_i2c() only performs the basic I2C initialization.
  - igb_set_i2c_bb() makes sure that E1000_CTRL_I2C_ENA is set
    and enables bit-banging.

- igb_probe() only calls igb_set_i2c_bb() if an ETS is present.

- igb_probe() calls init_thermal_sensor_thresh() unconditionally.

- igb_reset() aligns its behaviour to igb_probe(), i. e., call
  igb_set_i2c_bb() if an ETS is present and call
  init_thermal_sensor_thresh() unconditionally.

Fixes: a97f8783a937 ("igb: unbreak I2C bit-banging on i350")
Co-authored-by: Jamie Bainbridge <jbainbri@redhat.com>
Tested-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
Signed-off-by: Jamie Bainbridge <jbainbri@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 44 +++++++++++++++++------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 4e65ffe3f4e3..7f56322b3ec2 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -138,6 +138,9 @@ static irqreturn_t igb_msix_ring(int irq, void *);
 static void igb_update_dca(struct igb_q_vector *);
 static void igb_setup_dca(struct igb_adapter *);
 #endif /* CONFIG_IGB_DCA */
+#ifdef CONFIG_IGB_HWMON
+static void igb_set_i2c_bb(struct e1000_hw *);
+#endif /* CONFIG_IGB_HWMON */
 static int igb_poll(struct napi_struct *, int);
 static bool igb_clean_tx_irq(struct igb_q_vector *, int);
 static int igb_clean_rx_irq(struct igb_q_vector *, int);
@@ -2399,7 +2402,8 @@ void igb_reset(struct igb_adapter *adapter)
 			 * interface.
 			 */
 			if (adapter->ets)
-				mac->ops.init_thermal_sensor_thresh(hw);
+				igb_set_i2c_bb(hw);
+			mac->ops.init_thermal_sensor_thresh(hw);
 		}
 	}
 #endif
@@ -3116,21 +3120,12 @@ static void igb_init_mas(struct igb_adapter *adapter)
  **/
 static s32 igb_init_i2c(struct igb_adapter *adapter)
 {
-	struct e1000_hw *hw = &adapter->hw;
 	s32 status = 0;
-	s32 i2cctl;
 
 	/* I2C interface supported on i350 devices */
 	if (adapter->hw.mac.type != e1000_i350)
 		return 0;
 
-	i2cctl = rd32(E1000_I2CPARAMS);
-	i2cctl |= E1000_I2CBB_EN
-		| E1000_I2C_CLK_OUT | E1000_I2C_CLK_OE_N
-		| E1000_I2C_DATA_OUT | E1000_I2C_DATA_OE_N;
-	wr32(E1000_I2CPARAMS, i2cctl);
-	wrfl();
-
 	/* Initialize the i2c bus which is controlled by the registers.
 	 * This bus will use the i2c_algo_bit structure that implements
 	 * the protocol through toggling of the 4 bits in the register.
@@ -3146,6 +3141,30 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
 	return status;
 }
 
+#ifdef CONFIG_IGB_HWMON
+/**
+ *  igb_set_i2c_bb - Init I2C interface
+ *  @adapter: pointer to adapter structure
+ **/
+static void igb_set_i2c_bb(struct e1000_hw *hw)
+{
+	s32 i2cctl;
+	u32 ctrl_ext;
+
+	ctrl_ext = rd32(E1000_CTRL_EXT);
+	ctrl_ext |= E1000_CTRL_I2C_ENA;
+	wr32(E1000_CTRL_EXT, ctrl_ext);
+	wrfl();
+
+	i2cctl = rd32(E1000_I2CPARAMS);
+	i2cctl |= E1000_I2CBB_EN
+		| E1000_I2C_CLK_OE_N
+		| E1000_I2C_DATA_OE_N;
+	wr32(E1000_I2CPARAMS, i2cctl);
+	wrfl();
+}
+#endif
+
 /**
  *  igb_probe - Device Initialization Routine
  *  @pdev: PCI device information struct
@@ -3520,6 +3539,11 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			adapter->ets = true;
 		else
 			adapter->ets = false;
+		/* Only enable I2C bit banging if an external thermal
+		   sensor is supported. */
+		if (adapter->ets)
+		  igb_set_i2c_bb(hw);
+		hw->mac.ops.init_thermal_sensor_thresh(hw);
 		if (igb_sysfs_init(adapter))
 			dev_err(&pdev->dev,
 				"failed to allocate sysfs resources\n");
-- 
2.31.1

Re: [PATCH] igb: conditionalize I2C bit banging on external thermal sensor support

[Leon Romanovsky]

On Wed, Dec 07, 2022 at 11:49:07AM +0100, Corinna Vinschen wrote:
> Commit a97f8783a937 ("igb: unbreak I2C bit-banging on i350") introduced
> code to change I2C settings to bit banging unconditionally.
> 
> However, this patch introduced a regression:  On an Intel S2600CWR
> Server Board with three NICs:
> 
> - 1x dual-port copper
>   Intel I350 Gigabit Network Connection [8086:1521] (rev 01)
>   fw 1.63, 0x80000dda
> 
> - 2x quad-port SFP+ with copper SFP Avago ABCU-5700RZ
>   Intel I350 Gigabit Fiber Network Connection [8086:1522] (rev 01)
>   fw 1.52.0
> 
> the SFP NICs no longer get link at all.  Reverting commit a97f8783a937
> or switching to the Intel out-of-tree driver both fix the problem.
> 
> Per the igb out-of-tree driver, I2C bit banging on i350 depends on
> support for an external thermal sensor (ETS).  However, commit
> a97f8783a937 added bit banging unconditionally.  Additionally, the
> out-of-tree driver always calls init_thermal_sensor_thresh on probe,
> while our driver only calls init_thermal_sensor_thresh only in
> igb_reset(), and only if an ETS is present, ignoring the internal
> thermal sensor.  The affected SFPs don't provide an ETS.  Per Intel,
> the behaviour is a result of i350 firmware requirements.
> 
> This patch fixes the problem by aligning the behaviour to the
> out-of-tree driver:
> 
> - split igb_init_i2c() into two functions:
>   - igb_init_i2c() only performs the basic I2C initialization.
>   - igb_set_i2c_bb() makes sure that E1000_CTRL_I2C_ENA is set
>     and enables bit-banging.
> 
> - igb_probe() only calls igb_set_i2c_bb() if an ETS is present.
> 
> - igb_probe() calls init_thermal_sensor_thresh() unconditionally.
> 
> - igb_reset() aligns its behaviour to igb_probe(), i. e., call
>   igb_set_i2c_bb() if an ETS is present and call
>   init_thermal_sensor_thresh() unconditionally.
> 
> Fixes: a97f8783a937 ("igb: unbreak I2C bit-banging on i350")
> Co-authored-by: Jamie Bainbridge <jbainbri@redhat.com>
> Tested-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> Signed-off-by: Jamie Bainbridge <jbainbri@redhat.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 44 +++++++++++++++++------
>  1 file changed, 34 insertions(+), 10 deletions(-)

The patch should have target in its title: "[PATCH net] ...."

> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 4e65ffe3f4e3..7f56322b3ec2 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -138,6 +138,9 @@ static irqreturn_t igb_msix_ring(int irq, void *);
>  static void igb_update_dca(struct igb_q_vector *);
>  static void igb_setup_dca(struct igb_adapter *);
>  #endif /* CONFIG_IGB_DCA */
> +#ifdef CONFIG_IGB_HWMON
> +static void igb_set_i2c_bb(struct e1000_hw *);

IMHO, it is better to follow kernel coding style for new code.
The functions should have variable names too.

> +#endif /* CONFIG_IGB_HWMON */
>  static int igb_poll(struct napi_struct *, int);
>  static bool igb_clean_tx_irq(struct igb_q_vector *, int);
>  static int igb_clean_rx_irq(struct igb_q_vector *, int);
> @@ -2399,7 +2402,8 @@ void igb_reset(struct igb_adapter *adapter)
>  			 * interface.
>  			 */
>  			if (adapter->ets)
> -				mac->ops.init_thermal_sensor_thresh(hw);
> +				igb_set_i2c_bb(hw);
> +			mac->ops.init_thermal_sensor_thresh(hw);
>  		}
>  	}
>  #endif
> @@ -3116,21 +3120,12 @@ static void igb_init_mas(struct igb_adapter *adapter)
>   **/
>  static s32 igb_init_i2c(struct igb_adapter *adapter)
>  {
> -	struct e1000_hw *hw = &adapter->hw;
>  	s32 status = 0;
> -	s32 i2cctl;
>  
>  	/* I2C interface supported on i350 devices */
>  	if (adapter->hw.mac.type != e1000_i350)
>  		return 0;
>  
> -	i2cctl = rd32(E1000_I2CPARAMS);
> -	i2cctl |= E1000_I2CBB_EN
> -		| E1000_I2C_CLK_OUT | E1000_I2C_CLK_OE_N
> -		| E1000_I2C_DATA_OUT | E1000_I2C_DATA_OE_N;
> -	wr32(E1000_I2CPARAMS, i2cctl);
> -	wrfl();
> -
>  	/* Initialize the i2c bus which is controlled by the registers.
>  	 * This bus will use the i2c_algo_bit structure that implements
>  	 * the protocol through toggling of the 4 bits in the register.
> @@ -3146,6 +3141,30 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
>  	return status;
>  }
>  
> +#ifdef CONFIG_IGB_HWMON
> +/**
> + *  igb_set_i2c_bb - Init I2C interface
> + *  @adapter: pointer to adapter structure
> + **/
> +static void igb_set_i2c_bb(struct e1000_hw *hw)
> +{
> +	s32 i2cctl;
> +	u32 ctrl_ext;

Reversed Christmas tree, please.

Thanks

[PATCH net v2] igb: conditionalize I2C bit banging on external thermal sensor support

[Corinna Vinschen]

Commit a97f8783a937 ("igb: unbreak I2C bit-banging on i350") introduced
code to change I2C settings to bit banging unconditionally.

However, this patch introduced a regression:  On an Intel S2600CWR
Server Board with three NICs:

- 1x dual-port copper
  Intel I350 Gigabit Network Connection [8086:1521] (rev 01)
  fw 1.63, 0x80000dda

- 2x quad-port SFP+ with copper SFP Avago ABCU-5700RZ
  Intel I350 Gigabit Fiber Network Connection [8086:1522] (rev 01)
  fw 1.52.0

the SFP NICs no longer get link at all.  Reverting commit a97f8783a937
or switching to the Intel out-of-tree driver both fix the problem.

Per the igb out-of-tree driver, I2C bit banging on i350 depends on
support for an external thermal sensor (ETS).  However, commit
a97f8783a937 added bit banging unconditionally.  Additionally, the
out-of-tree driver always calls init_thermal_sensor_thresh on probe,
while our driver only calls init_thermal_sensor_thresh only in
igb_reset(), and only if an ETS is present, ignoring the internal
thermal sensor.  The affected SFPs don't provide an ETS.  Per Intel,
the behaviour is a result of i350 firmware requirements.

This patch fixes the problem by aligning the behaviour to the
out-of-tree driver:

- split igb_init_i2c() into two functions:
  - igb_init_i2c() only performs the basic I2C initialization.
  - igb_set_i2c_bb() makes sure that E1000_CTRL_I2C_ENA is set
    and enables bit-banging.

- igb_probe() only calls igb_set_i2c_bb() if an ETS is present.

- igb_probe() calls init_thermal_sensor_thresh() unconditionally.

- igb_reset() aligns its behaviour to igb_probe(), i. e., call
  igb_set_i2c_bb() if an ETS is present and call
  init_thermal_sensor_thresh() unconditionally.

v2: Add variable name in function declaration,
    rearrange declaration of local variables

Fixes: a97f8783a937 ("igb: unbreak I2C bit-banging on i350")
Co-authored-by: Jamie Bainbridge <jbainbri@redhat.com>
Tested-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
Signed-off-by: Jamie Bainbridge <jbainbri@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 44 +++++++++++++++++------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 4e65ffe3f4e3..7f56322b3ec2 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -138,6 +138,9 @@ static irqreturn_t igb_msix_ring(int irq, void *);
 static void igb_update_dca(struct igb_q_vector *);
 static void igb_setup_dca(struct igb_adapter *);
 #endif /* CONFIG_IGB_DCA */
+#ifdef CONFIG_IGB_HWMON
+static void igb_set_i2c_bb(struct e1000_hw *hw);
+#endif /* CONFIG_IGB_HWMON */
 static int igb_poll(struct napi_struct *, int);
 static bool igb_clean_tx_irq(struct igb_q_vector *, int);
 static int igb_clean_rx_irq(struct igb_q_vector *, int);
@@ -2399,7 +2402,8 @@ void igb_reset(struct igb_adapter *adapter)
 			 * interface.
 			 */
 			if (adapter->ets)
-				mac->ops.init_thermal_sensor_thresh(hw);
+				igb_set_i2c_bb(hw);
+			mac->ops.init_thermal_sensor_thresh(hw);
 		}
 	}
 #endif
@@ -3116,21 +3120,12 @@ static void igb_init_mas(struct igb_adapter *adapter)
  **/
 static s32 igb_init_i2c(struct igb_adapter *adapter)
 {
-	struct e1000_hw *hw = &adapter->hw;
 	s32 status = 0;
-	s32 i2cctl;
 
 	/* I2C interface supported on i350 devices */
 	if (adapter->hw.mac.type != e1000_i350)
 		return 0;
 
-	i2cctl = rd32(E1000_I2CPARAMS);
-	i2cctl |= E1000_I2CBB_EN
-		| E1000_I2C_CLK_OUT | E1000_I2C_CLK_OE_N
-		| E1000_I2C_DATA_OUT | E1000_I2C_DATA_OE_N;
-	wr32(E1000_I2CPARAMS, i2cctl);
-	wrfl();
-
 	/* Initialize the i2c bus which is controlled by the registers.
 	 * This bus will use the i2c_algo_bit structure that implements
 	 * the protocol through toggling of the 4 bits in the register.
@@ -3146,6 +3141,30 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
 	return status;
 }
 
+#ifdef CONFIG_IGB_HWMON
+/**
+ *  igb_set_i2c_bb - Init I2C interface
+ *  @adapter: pointer to adapter structure
+ **/
+static void igb_set_i2c_bb(struct e1000_hw *hw)
+{
+	u32 ctrl_ext;
+	s32 i2cctl;
+
+	ctrl_ext = rd32(E1000_CTRL_EXT);
+	ctrl_ext |= E1000_CTRL_I2C_ENA;
+	wr32(E1000_CTRL_EXT, ctrl_ext);
+	wrfl();
+
+	i2cctl = rd32(E1000_I2CPARAMS);
+	i2cctl |= E1000_I2CBB_EN
+		| E1000_I2C_CLK_OE_N
+		| E1000_I2C_DATA_OE_N;
+	wr32(E1000_I2CPARAMS, i2cctl);
+	wrfl();
+}
+#endif
+
 /**
  *  igb_probe - Device Initialization Routine
  *  @pdev: PCI device information struct
@@ -3520,6 +3539,11 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			adapter->ets = true;
 		else
 			adapter->ets = false;
+		/* Only enable I2C bit banging if an external thermal
+		   sensor is supported. */
+		if (adapter->ets)
+		  igb_set_i2c_bb(hw);
+		hw->mac.ops.init_thermal_sensor_thresh(hw);
 		if (igb_sysfs_init(adapter))
 			dev_err(&pdev->dev,
 				"failed to allocate sysfs resources\n");
-- 
2.31.1

Re: [Intel-wired-lan] [PATCH net v2] igb: conditionalize I2C bit banging on external thermal sensor support

[Tony Nguyen]

On 12/8/2022 3:21 AM, Corinna Vinschen wrote:
> Commit a97f8783a937 ("igb: unbreak I2C bit-banging on i350") introduced
> code to change I2C settings to bit banging unconditionally.
> 
> However, this patch introduced a regression:  On an Intel S2600CWR
> Server Board with three NICs:
> 
> - 1x dual-port copper
>    Intel I350 Gigabit Network Connection [8086:1521] (rev 01)
>    fw 1.63, 0x80000dda
> 
> - 2x quad-port SFP+ with copper SFP Avago ABCU-5700RZ
>    Intel I350 Gigabit Fiber Network Connection [8086:1522] (rev 01)
>    fw 1.52.0
> 
> the SFP NICs no longer get link at all.  Reverting commit a97f8783a937
> or switching to the Intel out-of-tree driver both fix the problem.
> 
> Per the igb out-of-tree driver, I2C bit banging on i350 depends on
> support for an external thermal sensor (ETS).  However, commit
> a97f8783a937 added bit banging unconditionally.  Additionally, the
> out-of-tree driver always calls init_thermal_sensor_thresh on probe,
> while our driver only calls init_thermal_sensor_thresh only in
> igb_reset(), and only if an ETS is present, ignoring the internal
> thermal sensor.  The affected SFPs don't provide an ETS.  Per Intel,
> the behaviour is a result of i350 firmware requirements.
> 
> This patch fixes the problem by aligning the behaviour to the
> out-of-tree driver:
> 
> - split igb_init_i2c() into two functions:
>    - igb_init_i2c() only performs the basic I2C initialization.
>    - igb_set_i2c_bb() makes sure that E1000_CTRL_I2C_ENA is set
>      and enables bit-banging.
> 
> - igb_probe() only calls igb_set_i2c_bb() if an ETS is present.
> 
> - igb_probe() calls init_thermal_sensor_thresh() unconditionally.
> 
> - igb_reset() aligns its behaviour to igb_probe(), i. e., call
>    igb_set_i2c_bb() if an ETS is present and call
>    init_thermal_sensor_thresh() unconditionally.
> 
> v2: Add variable name in function declaration,
>      rearrange declaration of local variables
> 
> Fixes: a97f8783a937 ("igb: unbreak I2C bit-banging on i350")
> Co-authored-by: Jamie Bainbridge <jbainbri@redhat.com>

Checkpatch doesn't like this: WARNING: Non-standard signature: 
Co-authored-by:

Co-developed-by: Jamie Bainbridge <jbainbri@redhat.com>
Signed-off-by: Jamie Bainbridge <jbainbri@redhat.com>

Would convey the same and keep checkpatch happy.

> Tested-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> Signed-off-by: Jamie Bainbridge <jbainbri@redhat.com>
> ---
>   drivers/net/ethernet/intel/igb/igb_main.c | 44 +++++++++++++++++------
>   1 file changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 4e65ffe3f4e3..7f56322b3ec2 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -138,6 +138,9 @@ static irqreturn_t igb_msix_ring(int irq, void *);
>   static void igb_update_dca(struct igb_q_vector *);
>   static void igb_setup_dca(struct igb_adapter *);
>   #endif /* CONFIG_IGB_DCA */
> +#ifdef CONFIG_IGB_HWMON
> +static void igb_set_i2c_bb(struct e1000_hw *hw);
> +#endif /* CONFIG_IGB_HWMON */

I believe the preference is to put the function in a place where the 
forward declaration isn't needed.

>   static int igb_poll(struct napi_struct *, int);
>   static bool igb_clean_tx_irq(struct igb_q_vector *, int);
>   static int igb_clean_rx_irq(struct igb_q_vector *, int);
> @@ -2399,7 +2402,8 @@ void igb_reset(struct igb_adapter *adapter)
>   			 * interface.
>   			 */
>   			if (adapter->ets)
> -				mac->ops.init_thermal_sensor_thresh(hw);
> +				igb_set_i2c_bb(hw);
> +			mac->ops.init_thermal_sensor_thresh(hw);
>   		}
>   	}
>   #endif
> @@ -3116,21 +3120,12 @@ static void igb_init_mas(struct igb_adapter *adapter)
>    **/
>   static s32 igb_init_i2c(struct igb_adapter *adapter)
>   {
> -	struct e1000_hw *hw = &adapter->hw;
>   	s32 status = 0;
> -	s32 i2cctl;
>   
>   	/* I2C interface supported on i350 devices */
>   	if (adapter->hw.mac.type != e1000_i350)
>   		return 0;
>   
> -	i2cctl = rd32(E1000_I2CPARAMS);
> -	i2cctl |= E1000_I2CBB_EN
> -		| E1000_I2C_CLK_OUT | E1000_I2C_CLK_OE_N
> -		| E1000_I2C_DATA_OUT | E1000_I2C_DATA_OE_N;
> -	wr32(E1000_I2CPARAMS, i2cctl);
> -	wrfl();
> -
>   	/* Initialize the i2c bus which is controlled by the registers.
>   	 * This bus will use the i2c_algo_bit structure that implements
>   	 * the protocol through toggling of the 4 bits in the register.
> @@ -3146,6 +3141,30 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
>   	return status;
>   }
>   
> +#ifdef CONFIG_IGB_HWMON
> +/**
> + *  igb_set_i2c_bb - Init I2C interface
> + *  @adapter: pointer to adapter structure

Copy/paste issue I assume. This needs to document hw, not adapter.

> + **/
> +static void igb_set_i2c_bb(struct e1000_hw *hw)
> +{
> +	u32 ctrl_ext;
> +	s32 i2cctl;
> +
> +	ctrl_ext = rd32(E1000_CTRL_EXT);
> +	ctrl_ext |= E1000_CTRL_I2C_ENA;
> +	wr32(E1000_CTRL_EXT, ctrl_ext);
> +	wrfl();
> +
> +	i2cctl = rd32(E1000_I2CPARAMS);
> +	i2cctl |= E1000_I2CBB_EN
> +		| E1000_I2C_CLK_OE_N
> +		| E1000_I2C_DATA_OE_N;
> +	wr32(E1000_I2CPARAMS, i2cctl);
> +	wrfl();
> +}
> +#endif
> +
>   /**
>    *  igb_probe - Device Initialization Routine
>    *  @pdev: PCI device information struct
> @@ -3520,6 +3539,11 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   			adapter->ets = true;
>   		else
>   			adapter->ets = false;
> +		/* Only enable I2C bit banging if an external thermal
> +		   sensor is supported. */

This comment style is incorrect.

/* Only enable I2C bit banging if an external thermal
  * sensor is supported.
  */

> +		if (adapter->ets)
> +		  igb_set_i2c_bb(hw);

Indentation is off: WARNING: suspect code indent for conditional 
statements (16, 18)

> +		hw->mac.ops.init_thermal_sensor_thresh(hw);
>   		if (igb_sysfs_init(adapter))
>   			dev_err(&pdev->dev,
>   				"failed to allocate sysfs resources\n");

[PATCH net v3] igb: conditionalize I2C bit banging on external thermal sensor support

[Corinna Vinschen]

Commit a97f8783a937 ("igb: unbreak I2C bit-banging on i350") introduced
code to change I2C settings to bit banging unconditionally.

However, this patch introduced a regression:  On an Intel S2600CWR
Server Board with three NICs:

- 1x dual-port copper
  Intel I350 Gigabit Network Connection [8086:1521] (rev 01)
  fw 1.63, 0x80000dda

- 2x quad-port SFP+ with copper SFP Avago ABCU-5700RZ
  Intel I350 Gigabit Fiber Network Connection [8086:1522] (rev 01)
  fw 1.52.0

the SFP NICs no longer get link at all.  Reverting commit a97f8783a937
or switching to the Intel out-of-tree driver both fix the problem.

Per the igb out-of-tree driver, I2C bit banging on i350 depends on
support for an external thermal sensor (ETS).  However, commit
a97f8783a937 added bit banging unconditionally.  Additionally, the
out-of-tree driver always calls init_thermal_sensor_thresh on probe,
while our driver only calls init_thermal_sensor_thresh only in
igb_reset(), and only if an ETS is present, ignoring the internal
thermal sensor.  The affected SFPs don't provide an ETS.  Per Intel,
the behaviour is a result of i350 firmware requirements.

This patch fixes the problem by aligning the behaviour to the
out-of-tree driver:

- split igb_init_i2c() into two functions:
  - igb_init_i2c() only performs the basic I2C initialization.
  - igb_set_i2c_bb() makes sure that E1000_CTRL_I2C_ENA is set
    and enables bit-banging.

- igb_probe() only calls igb_set_i2c_bb() if an ETS is present.

- igb_probe() calls init_thermal_sensor_thresh() unconditionally.

- igb_reset() aligns its behaviour to igb_probe(), i. e., call
  igb_set_i2c_bb() if an ETS is present and call
  init_thermal_sensor_thresh() unconditionally.

v2: Add variable name in function declaration,
    rearrange declaration of local variables
v3: Co-authored-by: -> Co-developed-by:
    Move igb_set_i2c_bb to avoid forward declaration
    Fix comments
    Fix indentation

Fixes: a97f8783a937 ("igb: unbreak I2C bit-banging on i350")
Tested-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Co-developed-by: Jamie Bainbridge <jbainbri@redhat.com>
Signed-off-by: Jamie Bainbridge <jbainbri@redhat.com>
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 42 +++++++++++++++++------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index f8e32833226c..1a68a3e91e7f 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2252,6 +2252,30 @@ static void igb_enable_mas(struct igb_adapter *adapter)
 	}
 }
 
+#ifdef CONFIG_IGB_HWMON
+/**
+ *  igb_set_i2c_bb - Init I2C interface
+ *  @hw: pointer to hardware structure
+ **/
+static void igb_set_i2c_bb(struct e1000_hw *hw)
+{
+	u32 ctrl_ext;
+	s32 i2cctl;
+
+	ctrl_ext = rd32(E1000_CTRL_EXT);
+	ctrl_ext |= E1000_CTRL_I2C_ENA;
+	wr32(E1000_CTRL_EXT, ctrl_ext);
+	wrfl();
+
+	i2cctl = rd32(E1000_I2CPARAMS);
+	i2cctl |= E1000_I2CBB_EN
+		| E1000_I2C_CLK_OE_N
+		| E1000_I2C_DATA_OE_N;
+	wr32(E1000_I2CPARAMS, i2cctl);
+	wrfl();
+}
+#endif
+
 void igb_reset(struct igb_adapter *adapter)
 {
 	struct pci_dev *pdev = adapter->pdev;
@@ -2396,7 +2420,8 @@ void igb_reset(struct igb_adapter *adapter)
 			 * interface.
 			 */
 			if (adapter->ets)
-				mac->ops.init_thermal_sensor_thresh(hw);
+				igb_set_i2c_bb(hw);
+			mac->ops.init_thermal_sensor_thresh(hw);
 		}
 	}
 #endif
@@ -3113,21 +3138,12 @@ static void igb_init_mas(struct igb_adapter *adapter)
  **/
 static s32 igb_init_i2c(struct igb_adapter *adapter)
 {
-	struct e1000_hw *hw = &adapter->hw;
 	s32 status = 0;
-	s32 i2cctl;
 
 	/* I2C interface supported on i350 devices */
 	if (adapter->hw.mac.type != e1000_i350)
 		return 0;
 
-	i2cctl = rd32(E1000_I2CPARAMS);
-	i2cctl |= E1000_I2CBB_EN
-		| E1000_I2C_CLK_OUT | E1000_I2C_CLK_OE_N
-		| E1000_I2C_DATA_OUT | E1000_I2C_DATA_OE_N;
-	wr32(E1000_I2CPARAMS, i2cctl);
-	wrfl();
-
 	/* Initialize the i2c bus which is controlled by the registers.
 	 * This bus will use the i2c_algo_bit structure that implements
 	 * the protocol through toggling of the 4 bits in the register.
@@ -3517,6 +3533,12 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			adapter->ets = true;
 		else
 			adapter->ets = false;
+		/* Only enable I2C bit banging if an external thermal
+		 * sensor is supported.
+		 */
+		if (adapter->ets)
+			igb_set_i2c_bb(hw);
+		hw->mac.ops.init_thermal_sensor_thresh(hw);
 		if (igb_sysfs_init(adapter))
 			dev_err(&pdev->dev,
 				"failed to allocate sysfs resources\n");
-- 
2.31.1