| Message ID | 20221208112104.2769660-1-vinschen@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [net,v2] igb: conditionalize I2C bit banging on external thermal sensor support | expand |
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");
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");
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(-)