| Message ID | 20240603-next-2024-06-03-intel-next-batch-v1-9-e0523b28f325@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Intel Wired LAN Driver Updates 2024-06-03 | expand |
On 6/3/2024 3:38 PM, Jacob Keller wrote: > > From: Vitaly Lifshits <vitaly.lifshits@intel.com> > > Add support for ethtool.set_phys_id callback to initiate LED blinking > and stopping them by the ethtool interface. > This is done by storing the initial LEDCTL register value and restoring > it when LED blinking is terminated. > > In addition, moved IGC_LEDCTL related defines from igc_leds.c to > igc_defines.h where they can be included by all of the igc module > files. > > Co-developed-by: Menachem Fogel <menachem.fogel@intel.com> > Signed-off-by: Menachem Fogel <menachem.fogel@intel.com> > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> > Tested-by: Naama Meir <naamax.meir@linux.intel.com> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/igc/igc_defines.h | 22 +++++++++++++++++++ > drivers/net/ethernet/intel/igc/igc_ethtool.c | 32 ++++++++++++++++++++++++++++ > drivers/net/ethernet/intel/igc/igc_hw.h | 2 ++ > drivers/net/ethernet/intel/igc/igc_leds.c | 21 +----------------- > drivers/net/ethernet/intel/igc/igc_main.c | 2 ++ > 5 files changed, 59 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h > index 5f92b3c7c3d4..664d49f10427 100644 > --- a/drivers/net/ethernet/intel/igc/igc_defines.h > +++ b/drivers/net/ethernet/intel/igc/igc_defines.h > @@ -686,4 +686,26 @@ > #define IGC_LTRMAXV_LSNP_REQ 0x00008000 /* LTR Snoop Requirement */ > #define IGC_LTRMAXV_SCALE_SHIFT 10 > > +/* LED ctrl defines */ > +#define IGC_NUM_LEDS 3 > + > +#define IGC_LEDCTL_GLOBAL_BLINK_MODE BIT(5) > +#define IGC_LEDCTL_LED0_MODE_SHIFT 0 > +#define IGC_LEDCTL_LED0_MODE_MASK GENMASK(3, 0) > +#define IGC_LEDCTL_LED0_BLINK BIT(7) > +#define IGC_LEDCTL_LED1_MODE_SHIFT 8 > +#define IGC_LEDCTL_LED1_MODE_MASK GENMASK(11, 8) > +#define IGC_LEDCTL_LED1_BLINK BIT(15) > +#define IGC_LEDCTL_LED2_MODE_SHIFT 16 > +#define IGC_LEDCTL_LED2_MODE_MASK GENMASK(19, 16) > +#define IGC_LEDCTL_LED2_BLINK BIT(23) > + > +#define IGC_LEDCTL_MODE_ON 0x00 > +#define IGC_LEDCTL_MODE_OFF 0x01 > +#define IGC_LEDCTL_MODE_LINK_10 0x05 > +#define IGC_LEDCTL_MODE_LINK_100 0x06 > +#define IGC_LEDCTL_MODE_LINK_1000 0x07 > +#define IGC_LEDCTL_MODE_LINK_2500 0x08 > +#define IGC_LEDCTL_MODE_ACTIVITY 0x0b > + > #endif /* _IGC_DEFINES_H_ */ > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c > index f2c4f1966bb0..82ece5f95f1e 100644 > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c > @@ -1975,6 +1975,37 @@ static void igc_ethtool_diag_test(struct net_device *netdev, > msleep_interruptible(4 * 1000); > } > > +static int igc_ethtool_set_phys_id(struct net_device *netdev, > + enum ethtool_phys_id_state state) > +{ > + struct igc_adapter *adapter = netdev_priv(netdev); > + struct igc_hw *hw = &adapter->hw; > + u32 ledctl; > + > + switch (state) { > + case ETHTOOL_ID_ACTIVE: > + ledctl = rd32(IGC_LEDCTL); > + > + /* initiate LED1 blinking */ > + ledctl &= ~(IGC_LEDCTL_GLOBAL_BLINK_MODE | > + IGC_LEDCTL_LED1_MODE_MASK | > + IGC_LEDCTL_LED2_MODE_MASK); > + ledctl |= IGC_LEDCTL_LED1_BLINK; > + wr32(IGC_LEDCTL, ledctl); > + break; > + > + case ETHTOOL_ID_INACTIVE: > + /* restore LEDCTL default value */ > + wr32(IGC_LEDCTL, hw->mac.ledctl_default); > + break; > + > + default: > + break; > + } > + > + return 0; > +} > + > static const struct ethtool_ops igc_ethtool_ops = { > .supported_coalesce_params = ETHTOOL_COALESCE_USECS, > .get_drvinfo = igc_ethtool_get_drvinfo, > @@ -2013,6 +2044,7 @@ static const struct ethtool_ops igc_ethtool_ops = { > .get_link_ksettings = igc_ethtool_get_link_ksettings, > .set_link_ksettings = igc_ethtool_set_link_ksettings, > .self_test = igc_ethtool_diag_test, > + .set_phys_id = igc_ethtool_set_phys_id, > }; > > void igc_ethtool_set_ops(struct net_device *netdev) > diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h > index e1c572e0d4ef..45b68695bdb7 100644 > --- a/drivers/net/ethernet/intel/igc/igc_hw.h > +++ b/drivers/net/ethernet/intel/igc/igc_hw.h > @@ -95,6 +95,8 @@ struct igc_mac_info { > bool autoneg; > bool autoneg_failed; > bool get_link_status; > + > + u32 ledctl_default; > }; > > struct igc_nvm_operations { > diff --git a/drivers/net/ethernet/intel/igc/igc_leds.c b/drivers/net/ethernet/intel/igc/igc_leds.c > index 3929b25b6ae6..e5eeef240802 100644 > --- a/drivers/net/ethernet/intel/igc/igc_leds.c > +++ b/drivers/net/ethernet/intel/igc/igc_leds.c > @@ -8,26 +8,7 @@ > #include <uapi/linux/uleds.h> > > #include "igc.h" > - > -#define IGC_NUM_LEDS 3 > - > -#define IGC_LEDCTL_LED0_MODE_SHIFT 0 > -#define IGC_LEDCTL_LED0_MODE_MASK GENMASK(3, 0) > -#define IGC_LEDCTL_LED0_BLINK BIT(7) > -#define IGC_LEDCTL_LED1_MODE_SHIFT 8 > -#define IGC_LEDCTL_LED1_MODE_MASK GENMASK(11, 8) > -#define IGC_LEDCTL_LED1_BLINK BIT(15) > -#define IGC_LEDCTL_LED2_MODE_SHIFT 16 > -#define IGC_LEDCTL_LED2_MODE_MASK GENMASK(19, 16) > -#define IGC_LEDCTL_LED2_BLINK BIT(23) > - > -#define IGC_LEDCTL_MODE_ON 0x00 > -#define IGC_LEDCTL_MODE_OFF 0x01 > -#define IGC_LEDCTL_MODE_LINK_10 0x05 > -#define IGC_LEDCTL_MODE_LINK_100 0x06 > -#define IGC_LEDCTL_MODE_LINK_1000 0x07 > -#define IGC_LEDCTL_MODE_LINK_2500 0x08 > -#define IGC_LEDCTL_MODE_ACTIVITY 0x0b > +#include "igc_defines.h" > > #define IGC_SUPPORTED_MODES \ > (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK_1000) | \ > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 12f004f46082..d0db302aa3eb 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -7070,6 +7070,8 @@ static int igc_probe(struct pci_dev *pdev, > goto err_register; > } > > + hw->mac.ledctl_default = rd32(IGC_LEDCTL); > + > return 0; Is this the only time the driver should read the register? Are there any other reasons/times that the LED register value might change while the driver is loaded that shouldn't get lost? If someone leaves the LED blinking then unloads the driver, is the LED left blinking? Should igc_remove() restore the default value? Thanks, sln > > err_register: > > -- > 2.44.0.53.g0f9d4d28b7e6 > >
On 6/3/2024 5:12 PM, Nelson, Shannon wrote: > On 6/3/2024 3:38 PM, Jacob Keller wrote: >> >> From: Vitaly Lifshits <vitaly.lifshits@intel.com> >> >> Add support for ethtool.set_phys_id callback to initiate LED blinking >> and stopping them by the ethtool interface. >> This is done by storing the initial LEDCTL register value and restoring >> it when LED blinking is terminated. >> >> In addition, moved IGC_LEDCTL related defines from igc_leds.c to >> igc_defines.h where they can be included by all of the igc module >> files. >> >> Co-developed-by: Menachem Fogel <menachem.fogel@intel.com> >> Signed-off-by: Menachem Fogel <menachem.fogel@intel.com> >> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> >> Tested-by: Naama Meir <naamax.meir@linux.intel.com> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >> --- >> drivers/net/ethernet/intel/igc/igc_defines.h | 22 +++++++++++++++++++ >> drivers/net/ethernet/intel/igc/igc_ethtool.c | 32 ++++++++++++++++++++++++++++ >> drivers/net/ethernet/intel/igc/igc_hw.h | 2 ++ >> drivers/net/ethernet/intel/igc/igc_leds.c | 21 +----------------- >> drivers/net/ethernet/intel/igc/igc_main.c | 2 ++ >> 5 files changed, 59 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h >> index 5f92b3c7c3d4..664d49f10427 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_defines.h >> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h >> @@ -686,4 +686,26 @@ >> #define IGC_LTRMAXV_LSNP_REQ 0x00008000 /* LTR Snoop Requirement */ >> #define IGC_LTRMAXV_SCALE_SHIFT 10 >> >> +/* LED ctrl defines */ >> +#define IGC_NUM_LEDS 3 >> + >> +#define IGC_LEDCTL_GLOBAL_BLINK_MODE BIT(5) >> +#define IGC_LEDCTL_LED0_MODE_SHIFT 0 >> +#define IGC_LEDCTL_LED0_MODE_MASK GENMASK(3, 0) >> +#define IGC_LEDCTL_LED0_BLINK BIT(7) >> +#define IGC_LEDCTL_LED1_MODE_SHIFT 8 >> +#define IGC_LEDCTL_LED1_MODE_MASK GENMASK(11, 8) >> +#define IGC_LEDCTL_LED1_BLINK BIT(15) >> +#define IGC_LEDCTL_LED2_MODE_SHIFT 16 >> +#define IGC_LEDCTL_LED2_MODE_MASK GENMASK(19, 16) >> +#define IGC_LEDCTL_LED2_BLINK BIT(23) >> + >> +#define IGC_LEDCTL_MODE_ON 0x00 >> +#define IGC_LEDCTL_MODE_OFF 0x01 >> +#define IGC_LEDCTL_MODE_LINK_10 0x05 >> +#define IGC_LEDCTL_MODE_LINK_100 0x06 >> +#define IGC_LEDCTL_MODE_LINK_1000 0x07 >> +#define IGC_LEDCTL_MODE_LINK_2500 0x08 >> +#define IGC_LEDCTL_MODE_ACTIVITY 0x0b >> + >> #endif /* _IGC_DEFINES_H_ */ >> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c >> index f2c4f1966bb0..82ece5f95f1e 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c >> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c >> @@ -1975,6 +1975,37 @@ static void igc_ethtool_diag_test(struct net_device *netdev, >> msleep_interruptible(4 * 1000); >> } >> >> +static int igc_ethtool_set_phys_id(struct net_device *netdev, >> + enum ethtool_phys_id_state state) >> +{ >> + struct igc_adapter *adapter = netdev_priv(netdev); >> + struct igc_hw *hw = &adapter->hw; >> + u32 ledctl; >> + >> + switch (state) { >> + case ETHTOOL_ID_ACTIVE: >> + ledctl = rd32(IGC_LEDCTL); >> + >> + /* initiate LED1 blinking */ >> + ledctl &= ~(IGC_LEDCTL_GLOBAL_BLINK_MODE | >> + IGC_LEDCTL_LED1_MODE_MASK | >> + IGC_LEDCTL_LED2_MODE_MASK); >> + ledctl |= IGC_LEDCTL_LED1_BLINK; >> + wr32(IGC_LEDCTL, ledctl); >> + break; >> + >> + case ETHTOOL_ID_INACTIVE: >> + /* restore LEDCTL default value */ >> + wr32(IGC_LEDCTL, hw->mac.ledctl_default); >> + break; >> + >> + default: >> + break; >> + } >> + >> + return 0; >> +} >> + >> static const struct ethtool_ops igc_ethtool_ops = { >> .supported_coalesce_params = ETHTOOL_COALESCE_USECS, >> .get_drvinfo = igc_ethtool_get_drvinfo, >> @@ -2013,6 +2044,7 @@ static const struct ethtool_ops igc_ethtool_ops = { >> .get_link_ksettings = igc_ethtool_get_link_ksettings, >> .set_link_ksettings = igc_ethtool_set_link_ksettings, >> .self_test = igc_ethtool_diag_test, >> + .set_phys_id = igc_ethtool_set_phys_id, >> }; >> >> void igc_ethtool_set_ops(struct net_device *netdev) >> diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h >> index e1c572e0d4ef..45b68695bdb7 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_hw.h >> +++ b/drivers/net/ethernet/intel/igc/igc_hw.h >> @@ -95,6 +95,8 @@ struct igc_mac_info { >> bool autoneg; >> bool autoneg_failed; >> bool get_link_status; >> + >> + u32 ledctl_default; >> }; >> >> struct igc_nvm_operations { >> diff --git a/drivers/net/ethernet/intel/igc/igc_leds.c b/drivers/net/ethernet/intel/igc/igc_leds.c >> index 3929b25b6ae6..e5eeef240802 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_leds.c >> +++ b/drivers/net/ethernet/intel/igc/igc_leds.c >> @@ -8,26 +8,7 @@ >> #include <uapi/linux/uleds.h> >> >> #include "igc.h" >> - >> -#define IGC_NUM_LEDS 3 >> - >> -#define IGC_LEDCTL_LED0_MODE_SHIFT 0 >> -#define IGC_LEDCTL_LED0_MODE_MASK GENMASK(3, 0) >> -#define IGC_LEDCTL_LED0_BLINK BIT(7) >> -#define IGC_LEDCTL_LED1_MODE_SHIFT 8 >> -#define IGC_LEDCTL_LED1_MODE_MASK GENMASK(11, 8) >> -#define IGC_LEDCTL_LED1_BLINK BIT(15) >> -#define IGC_LEDCTL_LED2_MODE_SHIFT 16 >> -#define IGC_LEDCTL_LED2_MODE_MASK GENMASK(19, 16) >> -#define IGC_LEDCTL_LED2_BLINK BIT(23) >> - >> -#define IGC_LEDCTL_MODE_ON 0x00 >> -#define IGC_LEDCTL_MODE_OFF 0x01 >> -#define IGC_LEDCTL_MODE_LINK_10 0x05 >> -#define IGC_LEDCTL_MODE_LINK_100 0x06 >> -#define IGC_LEDCTL_MODE_LINK_1000 0x07 >> -#define IGC_LEDCTL_MODE_LINK_2500 0x08 >> -#define IGC_LEDCTL_MODE_ACTIVITY 0x0b >> +#include "igc_defines.h" >> >> #define IGC_SUPPORTED_MODES \ >> (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK_1000) | \ >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index 12f004f46082..d0db302aa3eb 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -7070,6 +7070,8 @@ static int igc_probe(struct pci_dev *pdev, >> goto err_register; >> } >> >> + hw->mac.ledctl_default = rd32(IGC_LEDCTL); >> + >> return 0; > > Is this the only time the driver should read the register? Are there > any other reasons/times that the LED register value might change while > the driver is loaded that shouldn't get lost? > > If someone leaves the LED blinking then unloads the driver, is the LED > left blinking? Should igc_remove() restore the default value? > > Thanks, > sln > Good questions. Seems like to me that it would make more sense to initialize IGC_LEDCTL during probe to a known value, and possibly to ensure its cleared back to the normal state on driver remove. @Vitaly, any thoughts?
On Tue, Jun 04, 2024 at 02:12:31PM -0700, Jacob Keller wrote: > > > On 6/3/2024 5:12 PM, Nelson, Shannon wrote: > > On 6/3/2024 3:38 PM, Jacob Keller wrote: > >> > >> From: Vitaly Lifshits <vitaly.lifshits@intel.com> > >> > >> Add support for ethtool.set_phys_id callback to initiate LED blinking > >> and stopping them by the ethtool interface. > >> This is done by storing the initial LEDCTL register value and restoring > >> it when LED blinking is terminated. > >> > >> In addition, moved IGC_LEDCTL related defines from igc_leds.c to > >> igc_defines.h where they can be included by all of the igc module > >> files. Sorry for the deep nesting. I missed the first post. This seems like a very Intel specific solution to a very generic problem. The LED code added by Kurt Kanzenbach follows the generic netdev way of controlling LEDs. Any MAC or PHY driver with LED support should be capable of blinking. Maybe in hardware, maybe it needs software support. So please write a generic ethtool helper which any MAC driver can use to make use of the generic sys class LEDs associated to it, not an Intel specific solution. Andrew --- pw-bot: cr
On 6/4/2024 6:14 PM, Andrew Lunn wrote: > On Tue, Jun 04, 2024 at 02:12:31PM -0700, Jacob Keller wrote: >> >> >> On 6/3/2024 5:12 PM, Nelson, Shannon wrote: >>> On 6/3/2024 3:38 PM, Jacob Keller wrote: >>>> >>>> From: Vitaly Lifshits <vitaly.lifshits@intel.com> >>>> >>>> Add support for ethtool.set_phys_id callback to initiate LED blinking >>>> and stopping them by the ethtool interface. >>>> This is done by storing the initial LEDCTL register value and restoring >>>> it when LED blinking is terminated. >>>> >>>> In addition, moved IGC_LEDCTL related defines from igc_leds.c to >>>> igc_defines.h where they can be included by all of the igc module >>>> files. > > Sorry for the deep nesting. I missed the first post. > > This seems like a very Intel specific solution to a very generic > problem. The LED code added by Kurt Kanzenbach follows the generic > netdev way of controlling LEDs. Any MAC or PHY driver with LED support > should be capable of blinking. Maybe in hardware, maybe it needs > software support. > > So please write a generic ethtool helper which any MAC driver can use > to make use of the generic sys class LEDs associated to it, not an > Intel specific solution. > > Andrew ... Isn't that what the .set_phys_id ethtool callback is??? > * @set_phys_id: Identify the physical devices, e.g. by flashing an LED > * attached to it. The implementation may update the indicator > * asynchronously or synchronously, but in either case it must return > * quickly. It is initially called with the argument %ETHTOOL_ID_ACTIVE, > * and must either activate asynchronous updates and return zero, return > * a negative error or return a positive frequency for synchronous > * indication (e.g. 1 for one on/off cycle per second). If it returns > * a frequency then it will be called again at intervals with the > * argument %ETHTOOL_ID_ON or %ETHTOOL_ID_OFF and should set the state of > * the indicator accordingly. Finally, it is called with the argument > * %ETHTOOL_ID_INACTIVE and must deactivate the indicator. Returns a > * negative error code or zero. Maybe I'm misunderstanding here. Are you asking us to expose the LEDs via some other interface and extend ethtool to use that interface to blink LEDs?
> Maybe I'm misunderstanding here. Are you asking us to expose the LEDs > via some other interface and extend ethtool to use that interface to > blink LEDs? The LEDs are already exposed: commit ea578703b03d5d651b091c39f717dc829155b520 Author: Kurt Kanzenbach <kurt@linutronix.de> Date: Tue Feb 13 10:41:37 2024 -0800 igc: Add support for LEDs on i225/i226 Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs from user space using the netdev trigger. The LEDs are named as igc-<bus><device>-<led> to be easily identified. Offloading link speed and activity are supported. Other modes are simulated in software by using on/off. Tested on Intel i225. Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Tested-by: Naama Meir <naamax.meir@linux.intel.com> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> Link: https://lore.kernel.org/r/20240213184138.1483968-1-anthony.l.nguyen@intel.com Signed-off-by: Paolo Abeni <pabeni@redhat.com> The Linux LED subsystem knows about them. The Linux LED subsystem also knows about the qca8k LEDs, rtl8366rb LEDs, mediatek-ge-soc LEDs, air_en8811h LEDs, broadcom PHY LEDs, Marvell PHY LEDs, dp83867 LEDs, etc. There is nothing special here about igc. All these should be capable of blinking. So what i'm asking for is you add an ethtool helper, which implements this using the Linux LED subsystem to blink the LEDs. The same helper can then be used by other MAC drivers to blink their LEDs when they use the Linux LED subsystem. I'm sure there are a few different ways to implement this. One could be to extend the existing ledtrig-netdev.c. Add a call int netdev_trig_ethtool_phys_id(struct net_device *net_dev, enum ethtool_phys_id_state state) Which if passed ETHTOOL_ID_ON, ETHTOOL_ID_OFF, ETHTOOL_ID_ACTIVE, saves the current state and then sets the LEDs associated to the netdev to the requested state. If passed ETHTOOL_ID_INACTIVE it restores the previous state. Andrew
On 6/5/2024 1:31 PM, Andrew Lunn wrote: >> Maybe I'm misunderstanding here. Are you asking us to expose the LEDs >> via some other interface and extend ethtool to use that interface to >> blink LEDs? > > The LEDs are already exposed: > > commit ea578703b03d5d651b091c39f717dc829155b520 > Author: Kurt Kanzenbach <kurt@linutronix.de> > Date: Tue Feb 13 10:41:37 2024 -0800 > > igc: Add support for LEDs on i225/i226 > > Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs > from user space using the netdev trigger. The LEDs are named as > igc-<bus><device>-<led> to be easily identified. > > Offloading link speed and activity are supported. Other modes are simulated > in software by using on/off. Tested on Intel i225. > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Tested-by: Naama Meir <naamax.meir@linux.intel.com> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > Link: https://lore.kernel.org/r/20240213184138.1483968-1-anthony.l.nguyen@intel.com > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > The Linux LED subsystem knows about them. > > The Linux LED subsystem also knows about the qca8k LEDs, rtl8366rb > LEDs, mediatek-ge-soc LEDs, air_en8811h LEDs, broadcom PHY LEDs, > Marvell PHY LEDs, dp83867 LEDs, etc. There is nothing special here > about igc. All these should be capable of blinking. > > So what i'm asking for is you add an ethtool helper, which implements > this using the Linux LED subsystem to blink the LEDs. The same helper > can then be used by other MAC drivers to blink their LEDs when they > use the Linux LED subsystem. > > I'm sure there are a few different ways to implement this. One could > be to extend the existing ledtrig-netdev.c. Add a call > > int netdev_trig_ethtool_phys_id(struct net_device *net_dev, > enum ethtool_phys_id_state state) > > Which if passed ETHTOOL_ID_ON, ETHTOOL_ID_OFF, ETHTOOL_ID_ACTIVE, > saves the current state and then sets the LEDs associated to the > netdev to the requested state. If passed ETHTOOL_ID_INACTIVE it > restores the previous state. > > Andrew Makes sense, thanks for clarifying. @Vitaly, please look into this. Thanks, Jake
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h index 5f92b3c7c3d4..664d49f10427 100644 --- a/drivers/net/ethernet/intel/igc/igc_defines.h +++ b/drivers/net/ethernet/intel/igc/igc_defines.h @@ -686,4 +686,26 @@ #define IGC_LTRMAXV_LSNP_REQ 0x00008000 /* LTR Snoop Requirement */ #define IGC_LTRMAXV_SCALE_SHIFT 10 +/* LED ctrl defines */ +#define IGC_NUM_LEDS 3 + +#define IGC_LEDCTL_GLOBAL_BLINK_MODE BIT(5) +#define IGC_LEDCTL_LED0_MODE_SHIFT 0 +#define IGC_LEDCTL_LED0_MODE_MASK GENMASK(3, 0) +#define IGC_LEDCTL_LED0_BLINK BIT(7) +#define IGC_LEDCTL_LED1_MODE_SHIFT 8 +#define IGC_LEDCTL_LED1_MODE_MASK GENMASK(11, 8) +#define IGC_LEDCTL_LED1_BLINK BIT(15) +#define IGC_LEDCTL_LED2_MODE_SHIFT 16 +#define IGC_LEDCTL_LED2_MODE_MASK GENMASK(19, 16) +#define IGC_LEDCTL_LED2_BLINK BIT(23) + +#define IGC_LEDCTL_MODE_ON 0x00 +#define IGC_LEDCTL_MODE_OFF 0x01 +#define IGC_LEDCTL_MODE_LINK_10 0x05 +#define IGC_LEDCTL_MODE_LINK_100 0x06 +#define IGC_LEDCTL_MODE_LINK_1000 0x07 +#define IGC_LEDCTL_MODE_LINK_2500 0x08 +#define IGC_LEDCTL_MODE_ACTIVITY 0x0b + #endif /* _IGC_DEFINES_H_ */ diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c index f2c4f1966bb0..82ece5f95f1e 100644 --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c @@ -1975,6 +1975,37 @@ static void igc_ethtool_diag_test(struct net_device *netdev, msleep_interruptible(4 * 1000); } +static int igc_ethtool_set_phys_id(struct net_device *netdev, + enum ethtool_phys_id_state state) +{ + struct igc_adapter *adapter = netdev_priv(netdev); + struct igc_hw *hw = &adapter->hw; + u32 ledctl; + + switch (state) { + case ETHTOOL_ID_ACTIVE: + ledctl = rd32(IGC_LEDCTL); + + /* initiate LED1 blinking */ + ledctl &= ~(IGC_LEDCTL_GLOBAL_BLINK_MODE | + IGC_LEDCTL_LED1_MODE_MASK | + IGC_LEDCTL_LED2_MODE_MASK); + ledctl |= IGC_LEDCTL_LED1_BLINK; + wr32(IGC_LEDCTL, ledctl); + break; + + case ETHTOOL_ID_INACTIVE: + /* restore LEDCTL default value */ + wr32(IGC_LEDCTL, hw->mac.ledctl_default); + break; + + default: + break; + } + + return 0; +} + static const struct ethtool_ops igc_ethtool_ops = { .supported_coalesce_params = ETHTOOL_COALESCE_USECS, .get_drvinfo = igc_ethtool_get_drvinfo, @@ -2013,6 +2044,7 @@ static const struct ethtool_ops igc_ethtool_ops = { .get_link_ksettings = igc_ethtool_get_link_ksettings, .set_link_ksettings = igc_ethtool_set_link_ksettings, .self_test = igc_ethtool_diag_test, + .set_phys_id = igc_ethtool_set_phys_id, }; void igc_ethtool_set_ops(struct net_device *netdev) diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h index e1c572e0d4ef..45b68695bdb7 100644 --- a/drivers/net/ethernet/intel/igc/igc_hw.h +++ b/drivers/net/ethernet/intel/igc/igc_hw.h @@ -95,6 +95,8 @@ struct igc_mac_info { bool autoneg; bool autoneg_failed; bool get_link_status; + + u32 ledctl_default; }; struct igc_nvm_operations { diff --git a/drivers/net/ethernet/intel/igc/igc_leds.c b/drivers/net/ethernet/intel/igc/igc_leds.c index 3929b25b6ae6..e5eeef240802 100644 --- a/drivers/net/ethernet/intel/igc/igc_leds.c +++ b/drivers/net/ethernet/intel/igc/igc_leds.c @@ -8,26 +8,7 @@ #include <uapi/linux/uleds.h> #include "igc.h" - -#define IGC_NUM_LEDS 3 - -#define IGC_LEDCTL_LED0_MODE_SHIFT 0 -#define IGC_LEDCTL_LED0_MODE_MASK GENMASK(3, 0) -#define IGC_LEDCTL_LED0_BLINK BIT(7) -#define IGC_LEDCTL_LED1_MODE_SHIFT 8 -#define IGC_LEDCTL_LED1_MODE_MASK GENMASK(11, 8) -#define IGC_LEDCTL_LED1_BLINK BIT(15) -#define IGC_LEDCTL_LED2_MODE_SHIFT 16 -#define IGC_LEDCTL_LED2_MODE_MASK GENMASK(19, 16) -#define IGC_LEDCTL_LED2_BLINK BIT(23) - -#define IGC_LEDCTL_MODE_ON 0x00 -#define IGC_LEDCTL_MODE_OFF 0x01 -#define IGC_LEDCTL_MODE_LINK_10 0x05 -#define IGC_LEDCTL_MODE_LINK_100 0x06 -#define IGC_LEDCTL_MODE_LINK_1000 0x07 -#define IGC_LEDCTL_MODE_LINK_2500 0x08 -#define IGC_LEDCTL_MODE_ACTIVITY 0x0b +#include "igc_defines.h" #define IGC_SUPPORTED_MODES \ (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK_1000) | \ diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 12f004f46082..d0db302aa3eb 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -7070,6 +7070,8 @@ static int igc_probe(struct pci_dev *pdev, goto err_register; } + hw->mac.ledctl_default = rd32(IGC_LEDCTL); + return 0; err_register: