| Message ID | 20250207-marvell-88q2xxx-leds-v2-1-d0034e79e19d@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx | expand |
On Fri, Feb 07, 2025 at 05:24:20PM +0100, Dimitri Fedrau wrote: > Marvell 88Q2XXX devices support up to two configurable Light Emitting > Diode (LED). Add minimal LED controller driver supporting the most common > uses with the 'netdev' trigger. > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> Reviewed-by: Stefan Eichenberger <eichest@gmail.com> > --- > Changes in v2: > - Renamed MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK to > MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK (Stefan) > - Renamed MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK to > MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK (Stefan) > - Added comment for disabling tx disable feature (Stefan) > - Added defines for all led functions (Andrew) > - Move enabling of led0 function from mv88q2xxx_probe to > mv88q222x_config_init. When the hardware reset pin is connected to a GPIO > for example and we bring the interface down and up again, the content > of the register MDIO_MMD_PCS_MV_RESET_CTRL is resetted to default. > This means LED function is disabled and can't be enabled again. > - Link to v1: https://lore.kernel.org/r/20250110-marvell-88q2xxx-leds-v1-1-22e7734941c2@gmail.com > --- > drivers/net/phy/marvell-88q2xxx.c | 175 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 175 insertions(+) > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..2d5ea3e1b26219bb1e050222347c2688903e2430 100644 > --- a/drivers/net/phy/marvell-88q2xxx.c > +++ b/drivers/net/phy/marvell-88q2xxx.c > @@ -8,6 +8,7 @@ > */ > #include <linux/ethtool_netlink.h> > #include <linux/marvell_phy.h> > +#include <linux/of.h> > #include <linux/phy.h> > #include <linux/hwmon.h> > > @@ -27,6 +28,9 @@ > #define MDIO_MMD_AN_MV_STAT2_100BT1 0x2000 > #define MDIO_MMD_AN_MV_STAT2_1000BT1 0x4000 > > +#define MDIO_MMD_PCS_MV_RESET_CTRL 32768 > +#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE 0x8 > + > #define MDIO_MMD_PCS_MV_INT_EN 32784 > #define MDIO_MMD_PCS_MV_INT_EN_LINK_UP 0x0040 > #define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN 0x0080 > @@ -40,6 +44,22 @@ > #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL 32787 > #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS 0x0800 > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL 32790 > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK GENMASK(7, 4) > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK GENMASK(3, 0) > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK 0x0 /* Link established */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX 0x1 /* Link established, blink for rx or tx activity */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1 0x2 /* Blink 3x for 1000BT1 link established */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX_ON 0x3 /* Receive or transmit activity */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX 0x4 /* Blink on receive or transmit activity */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX 0x5 /* Transmit activity */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_COPPER 0x6 /* Copper Link established */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON 0x7 /* 1000BT1 link established */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_OFF 0x8 /* Force off */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_ON 0x9 /* Force on */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_HIGHZ 0xa /* Force Hi-Z */ > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_BLINK 0xb /* Force blink */ > + > #define MDIO_MMD_PCS_MV_TEMP_SENSOR1 32833 > #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT 0x0001 > #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT 0x0040 > @@ -95,8 +115,12 @@ > > #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF 65246 > > +#define MV88Q2XXX_LED_INDEX_TX_ENABLE 0 > +#define MV88Q2XXX_LED_INDEX_GPIO 1 > + > struct mv88q2xxx_priv { > bool enable_temp; > + bool enable_led0; > }; > > struct mmd_val { > @@ -740,15 +764,62 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev) > } > #endif > > +#if IS_ENABLED(CONFIG_OF_MDIO) > +static int mv88q2xxx_leds_probe(struct phy_device *phydev) > +{ > + struct device_node *node = phydev->mdio.dev.of_node; > + struct mv88q2xxx_priv *priv = phydev->priv; > + struct device_node *leds; > + int ret = 0; > + u32 index; > + > + if (!node) > + return 0; > + > + leds = of_get_child_by_name(node, "leds"); > + if (!leds) > + return 0; > + > + for_each_available_child_of_node_scoped(leds, led) { > + ret = of_property_read_u32(led, "reg", &index); > + if (ret) > + goto exit; > + > + if (index > MV88Q2XXX_LED_INDEX_GPIO) { > + ret = -EINVAL; > + goto exit; > + } > + > + if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE) > + priv->enable_led0 = true; > + } > + > +exit: > + of_node_put(leds); > + > + return ret; > +} > + > +#else > +static int mv88q2xxx_leds_probe(struct phy_device *phydev) > +{ > + return 0; > +} > +#endif > + > static int mv88q2xxx_probe(struct phy_device *phydev) > { > struct mv88q2xxx_priv *priv; > + int ret; > > priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; > > phydev->priv = priv; > + ret = mv88q2xxx_leds_probe(phydev); > + if (ret) > + return ret; > > return mv88q2xxx_hwmon_probe(phydev); > } > @@ -829,6 +900,15 @@ static int mv88q222x_config_init(struct phy_device *phydev) > return ret; > } > > + /* Enable LED function and disable TX disable feature on LED/TX_ENABLE */ > + if (priv->enable_led0) { > + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS, > + MDIO_MMD_PCS_MV_RESET_CTRL, > + MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE); > + if (ret < 0) > + return ret; > + } > + > if (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] == PHY_ID_88Q2220_REVB0) > return mv88q222x_revb0_config_init(phydev); > else > @@ -918,6 +998,98 @@ static int mv88q222x_cable_test_get_status(struct phy_device *phydev, > return 0; > } > > +static int mv88q2xxx_led_mode(u8 index, unsigned long rules) > +{ > + switch (rules) { > + case BIT(TRIGGER_NETDEV_LINK): > + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK; > + case BIT(TRIGGER_NETDEV_LINK_1000): > + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON; > + case BIT(TRIGGER_NETDEV_TX): > + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX; > + case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): > + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX; > + case BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): > + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX; > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int mv88q2xxx_led_hw_is_supported(struct phy_device *phydev, u8 index, > + unsigned long rules) > +{ > + int mode; > + > + mode = mv88q2xxx_led_mode(index, rules); > + if (mode < 0) > + return mode; > + > + return 0; > +} > + > +static int mv88q2xxx_led_hw_control_set(struct phy_device *phydev, u8 index, > + unsigned long rules) > +{ > + int mode; > + > + mode = mv88q2xxx_led_mode(index, rules); > + if (mode < 0) > + return mode; > + > + if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE) > + return phy_modify_mmd(phydev, MDIO_MMD_PCS, > + MDIO_MMD_PCS_MV_LED_FUNC_CTRL, > + MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK, > + FIELD_PREP(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK, > + mode)); > + else > + return phy_modify_mmd(phydev, MDIO_MMD_PCS, > + MDIO_MMD_PCS_MV_LED_FUNC_CTRL, > + MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK, > + FIELD_PREP(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK, > + mode)); > +} > + > +static int mv88q2xxx_led_hw_control_get(struct phy_device *phydev, u8 index, > + unsigned long *rules) > +{ > + int val; > + > + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_LED_FUNC_CTRL); > + if (val < 0) > + return val; > + > + if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE) > + val = FIELD_GET(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK, val); > + else > + val = FIELD_GET(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK, val); > + > + switch (val) { > + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK: > + *rules = BIT(TRIGGER_NETDEV_LINK); > + break; > + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON: > + *rules = BIT(TRIGGER_NETDEV_LINK_1000); > + break; > + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX: > + *rules = BIT(TRIGGER_NETDEV_TX); > + break; > + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX: > + *rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX); > + break; > + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX: > + *rules = BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | > + BIT(TRIGGER_NETDEV_RX); > + break; > + default: > + *rules = 0; > + break; > + } > + > + return 0; > +} > + > static struct phy_driver mv88q2xxx_driver[] = { > { > .phy_id = MARVELL_PHY_ID_88Q2110, > @@ -953,6 +1125,9 @@ static struct phy_driver mv88q2xxx_driver[] = { > .get_sqi_max = mv88q2xxx_get_sqi_max, > .suspend = mv88q2xxx_suspend, > .resume = mv88q2xxx_resume, > + .led_hw_is_supported = mv88q2xxx_led_hw_is_supported, > + .led_hw_control_set = mv88q2xxx_led_hw_control_set, > + .led_hw_control_get = mv88q2xxx_led_hw_control_get, > }, > }; > > > --- > base-commit: f84db3bc8abc2141839cdb9454061633a4ce1db7 > change-id: 20241221-marvell-88q2xxx-leds-69a4037b5157 > > Best regards, > -- > Dimitri Fedrau <dima.fedrau@gmail.com> >
Hi Stefan, Am Sat, Feb 08, 2025 at 05:44:26PM +0100 schrieb Stefan Eichenberger: > On Fri, Feb 07, 2025 at 05:24:20PM +0100, Dimitri Fedrau wrote: > > Marvell 88Q2XXX devices support up to two configurable Light Emitting > > Diode (LED). Add minimal LED controller driver supporting the most common > > uses with the 'netdev' trigger. > > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > Reviewed-by: Stefan Eichenberger <eichest@gmail.com> > thanks for reviewing. I just noticed that led0 is enabled in mv88q222x_config_init, but I think it should be enabled in mv88q2xxx_config_init because LED configuration is same for all mv88q2xxx devices. What do you think ? > > --- > > Changes in v2: > > - Renamed MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK to > > MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK (Stefan) > > - Renamed MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK to > > MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK (Stefan) > > - Added comment for disabling tx disable feature (Stefan) > > - Added defines for all led functions (Andrew) > > - Move enabling of led0 function from mv88q2xxx_probe to > > mv88q222x_config_init. When the hardware reset pin is connected to a GPIO > > for example and we bring the interface down and up again, the content > > of the register MDIO_MMD_PCS_MV_RESET_CTRL is resetted to default. > > This means LED function is disabled and can't be enabled again. > > - Link to v1: https://lore.kernel.org/r/20250110-marvell-88q2xxx-leds-v1-1-22e7734941c2@gmail.com > > --- > > drivers/net/phy/marvell-88q2xxx.c | 175 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 175 insertions(+) > > > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > > index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..2d5ea3e1b26219bb1e050222347c2688903e2430 100644 > > --- a/drivers/net/phy/marvell-88q2xxx.c > > +++ b/drivers/net/phy/marvell-88q2xxx.c > > @@ -8,6 +8,7 @@ > > */ > > #include <linux/ethtool_netlink.h> > > #include <linux/marvell_phy.h> > > +#include <linux/of.h> > > #include <linux/phy.h> > > #include <linux/hwmon.h> > > > > @@ -27,6 +28,9 @@ > > #define MDIO_MMD_AN_MV_STAT2_100BT1 0x2000 > > #define MDIO_MMD_AN_MV_STAT2_1000BT1 0x4000 > > > > +#define MDIO_MMD_PCS_MV_RESET_CTRL 32768 > > +#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE 0x8 > > + > > #define MDIO_MMD_PCS_MV_INT_EN 32784 > > #define MDIO_MMD_PCS_MV_INT_EN_LINK_UP 0x0040 > > #define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN 0x0080 > > @@ -40,6 +44,22 @@ > > #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL 32787 > > #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS 0x0800 > > > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL 32790 > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK GENMASK(7, 4) > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK GENMASK(3, 0) > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK 0x0 /* Link established */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX 0x1 /* Link established, blink for rx or tx activity */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1 0x2 /* Blink 3x for 1000BT1 link established */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX_ON 0x3 /* Receive or transmit activity */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX 0x4 /* Blink on receive or transmit activity */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX 0x5 /* Transmit activity */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_COPPER 0x6 /* Copper Link established */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON 0x7 /* 1000BT1 link established */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_OFF 0x8 /* Force off */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_ON 0x9 /* Force on */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_HIGHZ 0xa /* Force Hi-Z */ > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_BLINK 0xb /* Force blink */ > > + > > #define MDIO_MMD_PCS_MV_TEMP_SENSOR1 32833 > > #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT 0x0001 > > #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT 0x0040 > > @@ -95,8 +115,12 @@ > > > > #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF 65246 > > > > +#define MV88Q2XXX_LED_INDEX_TX_ENABLE 0 > > +#define MV88Q2XXX_LED_INDEX_GPIO 1 > > + > > struct mv88q2xxx_priv { > > bool enable_temp; > > + bool enable_led0; > > }; > > > > struct mmd_val { > > @@ -740,15 +764,62 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev) > > } > > #endif > > > > +#if IS_ENABLED(CONFIG_OF_MDIO) > > +static int mv88q2xxx_leds_probe(struct phy_device *phydev) > > +{ > > + struct device_node *node = phydev->mdio.dev.of_node; > > + struct mv88q2xxx_priv *priv = phydev->priv; > > + struct device_node *leds; > > + int ret = 0; > > + u32 index; > > + > > + if (!node) > > + return 0; > > + > > + leds = of_get_child_by_name(node, "leds"); > > + if (!leds) > > + return 0; > > + > > + for_each_available_child_of_node_scoped(leds, led) { > > + ret = of_property_read_u32(led, "reg", &index); > > + if (ret) > > + goto exit; > > + > > + if (index > MV88Q2XXX_LED_INDEX_GPIO) { > > + ret = -EINVAL; > > + goto exit; > > + } > > + > > + if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE) > > + priv->enable_led0 = true; > > + } > > + > > +exit: > > + of_node_put(leds); > > + > > + return ret; > > +} > > + > > +#else > > +static int mv88q2xxx_leds_probe(struct phy_device *phydev) > > +{ > > + return 0; > > +} > > +#endif > > + > > static int mv88q2xxx_probe(struct phy_device *phydev) > > { > > struct mv88q2xxx_priv *priv; > > + int ret; > > > > priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL); > > if (!priv) > > return -ENOMEM; > > > > phydev->priv = priv; > > + ret = mv88q2xxx_leds_probe(phydev); > > + if (ret) > > + return ret; > > > > return mv88q2xxx_hwmon_probe(phydev); > > } > > @@ -829,6 +900,15 @@ static int mv88q222x_config_init(struct phy_device *phydev) > > return ret; > > } > > > > + /* Enable LED function and disable TX disable feature on LED/TX_ENABLE */ > > + if (priv->enable_led0) { > > + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS, > > + MDIO_MMD_PCS_MV_RESET_CTRL, > > + MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE); > > + if (ret < 0) > > + return ret; > > + } > > + > > if (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] == PHY_ID_88Q2220_REVB0) > > return mv88q222x_revb0_config_init(phydev); > > else > > @@ -918,6 +998,98 @@ static int mv88q222x_cable_test_get_status(struct phy_device *phydev, > > return 0; > > } > > > > +static int mv88q2xxx_led_mode(u8 index, unsigned long rules) > > +{ > > + switch (rules) { > > + case BIT(TRIGGER_NETDEV_LINK): > > + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK; > > + case BIT(TRIGGER_NETDEV_LINK_1000): > > + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON; > > + case BIT(TRIGGER_NETDEV_TX): > > + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX; > > + case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): > > + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX; > > + case BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): > > + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX; > > + default: > > + return -EOPNOTSUPP; > > + } > > +} > > + > > +static int mv88q2xxx_led_hw_is_supported(struct phy_device *phydev, u8 index, > > + unsigned long rules) > > +{ > > + int mode; > > + > > + mode = mv88q2xxx_led_mode(index, rules); > > + if (mode < 0) > > + return mode; > > + > > + return 0; > > +} > > + > > +static int mv88q2xxx_led_hw_control_set(struct phy_device *phydev, u8 index, > > + unsigned long rules) > > +{ > > + int mode; > > + > > + mode = mv88q2xxx_led_mode(index, rules); > > + if (mode < 0) > > + return mode; > > + > > + if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE) > > + return phy_modify_mmd(phydev, MDIO_MMD_PCS, > > + MDIO_MMD_PCS_MV_LED_FUNC_CTRL, > > + MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK, > > + FIELD_PREP(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK, > > + mode)); > > + else > > + return phy_modify_mmd(phydev, MDIO_MMD_PCS, > > + MDIO_MMD_PCS_MV_LED_FUNC_CTRL, > > + MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK, > > + FIELD_PREP(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK, > > + mode)); > > +} > > + > > +static int mv88q2xxx_led_hw_control_get(struct phy_device *phydev, u8 index, > > + unsigned long *rules) > > +{ > > + int val; > > + > > + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_LED_FUNC_CTRL); > > + if (val < 0) > > + return val; > > + > > + if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE) > > + val = FIELD_GET(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK, val); > > + else > > + val = FIELD_GET(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK, val); > > + > > + switch (val) { > > + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK: > > + *rules = BIT(TRIGGER_NETDEV_LINK); > > + break; > > + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON: > > + *rules = BIT(TRIGGER_NETDEV_LINK_1000); > > + break; > > + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX: > > + *rules = BIT(TRIGGER_NETDEV_TX); > > + break; > > + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX: > > + *rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX); > > + break; > > + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX: > > + *rules = BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | > > + BIT(TRIGGER_NETDEV_RX); > > + break; > > + default: > > + *rules = 0; > > + break; > > + } > > + > > + return 0; > > +} > > + > > static struct phy_driver mv88q2xxx_driver[] = { > > { > > .phy_id = MARVELL_PHY_ID_88Q2110, > > @@ -953,6 +1125,9 @@ static struct phy_driver mv88q2xxx_driver[] = { > > .get_sqi_max = mv88q2xxx_get_sqi_max, > > .suspend = mv88q2xxx_suspend, > > .resume = mv88q2xxx_resume, > > + .led_hw_is_supported = mv88q2xxx_led_hw_is_supported, > > + .led_hw_control_set = mv88q2xxx_led_hw_control_set, > > + .led_hw_control_get = mv88q2xxx_led_hw_control_get, > > }, > > }; > > > > > > --- > > base-commit: f84db3bc8abc2141839cdb9454061633a4ce1db7 > > change-id: 20241221-marvell-88q2xxx-leds-69a4037b5157 > > > > Best regards, > > -- > > Dimitri Fedrau <dima.fedrau@gmail.com> > >
On Sun, Feb 09, 2025 at 09:41:35AM +0100, Dimitri Fedrau wrote: > Hi Stefan, > > Am Sat, Feb 08, 2025 at 05:44:26PM +0100 schrieb Stefan Eichenberger: > > On Fri, Feb 07, 2025 at 05:24:20PM +0100, Dimitri Fedrau wrote: > > > Marvell 88Q2XXX devices support up to two configurable Light Emitting > > > Diode (LED). Add minimal LED controller driver supporting the most common > > > uses with the 'netdev' trigger. > > > > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > > > Reviewed-by: Stefan Eichenberger <eichest@gmail.com> > > > > thanks for reviewing. I just noticed that led0 is enabled in > mv88q222x_config_init, but I think it should be enabled in > mv88q2xxx_config_init because LED configuration is same for all > mv88q2xxx devices. What do you think ? The code looks O.K. to me, so please add my Reviewed-by: Andrew Lunn <andrew@lunn.ch> if you decide to change to the generic 2xxx. Andrew
Hi Dimitri, On Sun, Feb 09, 2025 at 09:41:35AM +0100, Dimitri Fedrau wrote: > Hi Stefan, > > Am Sat, Feb 08, 2025 at 05:44:26PM +0100 schrieb Stefan Eichenberger: > > On Fri, Feb 07, 2025 at 05:24:20PM +0100, Dimitri Fedrau wrote: > > > Marvell 88Q2XXX devices support up to two configurable Light Emitting > > > Diode (LED). Add minimal LED controller driver supporting the most common > > > uses with the 'netdev' trigger. > > > > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > > > Reviewed-by: Stefan Eichenberger <eichest@gmail.com> > > > > thanks for reviewing. I just noticed that led0 is enabled in > mv88q222x_config_init, but I think it should be enabled in > mv88q2xxx_config_init because LED configuration is same for all > mv88q2xxx devices. What do you think ? I think you are right. If you can move that to mv88q2xxx_config_init it would be great. You can add my Reviewed-by tag to the updated patch. Thanks, Stefan
diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..2d5ea3e1b26219bb1e050222347c2688903e2430 100644 --- a/drivers/net/phy/marvell-88q2xxx.c +++ b/drivers/net/phy/marvell-88q2xxx.c @@ -8,6 +8,7 @@ */ #include <linux/ethtool_netlink.h> #include <linux/marvell_phy.h> +#include <linux/of.h> #include <linux/phy.h> #include <linux/hwmon.h> @@ -27,6 +28,9 @@ #define MDIO_MMD_AN_MV_STAT2_100BT1 0x2000 #define MDIO_MMD_AN_MV_STAT2_1000BT1 0x4000 +#define MDIO_MMD_PCS_MV_RESET_CTRL 32768 +#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE 0x8 + #define MDIO_MMD_PCS_MV_INT_EN 32784 #define MDIO_MMD_PCS_MV_INT_EN_LINK_UP 0x0040 #define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN 0x0080 @@ -40,6 +44,22 @@ #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL 32787 #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS 0x0800 +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL 32790 +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK GENMASK(7, 4) +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK GENMASK(3, 0) +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK 0x0 /* Link established */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX 0x1 /* Link established, blink for rx or tx activity */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1 0x2 /* Blink 3x for 1000BT1 link established */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX_ON 0x3 /* Receive or transmit activity */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX 0x4 /* Blink on receive or transmit activity */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX 0x5 /* Transmit activity */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_COPPER 0x6 /* Copper Link established */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON 0x7 /* 1000BT1 link established */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_OFF 0x8 /* Force off */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_ON 0x9 /* Force on */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_HIGHZ 0xa /* Force Hi-Z */ +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_FORCE_BLINK 0xb /* Force blink */ + #define MDIO_MMD_PCS_MV_TEMP_SENSOR1 32833 #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT 0x0001 #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT 0x0040 @@ -95,8 +115,12 @@ #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF 65246 +#define MV88Q2XXX_LED_INDEX_TX_ENABLE 0 +#define MV88Q2XXX_LED_INDEX_GPIO 1 + struct mv88q2xxx_priv { bool enable_temp; + bool enable_led0; }; struct mmd_val { @@ -740,15 +764,62 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev) } #endif +#if IS_ENABLED(CONFIG_OF_MDIO) +static int mv88q2xxx_leds_probe(struct phy_device *phydev) +{ + struct device_node *node = phydev->mdio.dev.of_node; + struct mv88q2xxx_priv *priv = phydev->priv; + struct device_node *leds; + int ret = 0; + u32 index; + + if (!node) + return 0; + + leds = of_get_child_by_name(node, "leds"); + if (!leds) + return 0; + + for_each_available_child_of_node_scoped(leds, led) { + ret = of_property_read_u32(led, "reg", &index); + if (ret) + goto exit; + + if (index > MV88Q2XXX_LED_INDEX_GPIO) { + ret = -EINVAL; + goto exit; + } + + if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE) + priv->enable_led0 = true; + } + +exit: + of_node_put(leds); + + return ret; +} + +#else +static int mv88q2xxx_leds_probe(struct phy_device *phydev) +{ + return 0; +} +#endif + static int mv88q2xxx_probe(struct phy_device *phydev) { struct mv88q2xxx_priv *priv; + int ret; priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; phydev->priv = priv; + ret = mv88q2xxx_leds_probe(phydev); + if (ret) + return ret; return mv88q2xxx_hwmon_probe(phydev); } @@ -829,6 +900,15 @@ static int mv88q222x_config_init(struct phy_device *phydev) return ret; } + /* Enable LED function and disable TX disable feature on LED/TX_ENABLE */ + if (priv->enable_led0) { + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS, + MDIO_MMD_PCS_MV_RESET_CTRL, + MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE); + if (ret < 0) + return ret; + } + if (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] == PHY_ID_88Q2220_REVB0) return mv88q222x_revb0_config_init(phydev); else @@ -918,6 +998,98 @@ static int mv88q222x_cable_test_get_status(struct phy_device *phydev, return 0; } +static int mv88q2xxx_led_mode(u8 index, unsigned long rules) +{ + switch (rules) { + case BIT(TRIGGER_NETDEV_LINK): + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK; + case BIT(TRIGGER_NETDEV_LINK_1000): + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON; + case BIT(TRIGGER_NETDEV_TX): + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX; + case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX; + case BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): + return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX; + default: + return -EOPNOTSUPP; + } +} + +static int mv88q2xxx_led_hw_is_supported(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + int mode; + + mode = mv88q2xxx_led_mode(index, rules); + if (mode < 0) + return mode; + + return 0; +} + +static int mv88q2xxx_led_hw_control_set(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + int mode; + + mode = mv88q2xxx_led_mode(index, rules); + if (mode < 0) + return mode; + + if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE) + return phy_modify_mmd(phydev, MDIO_MMD_PCS, + MDIO_MMD_PCS_MV_LED_FUNC_CTRL, + MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK, + FIELD_PREP(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK, + mode)); + else + return phy_modify_mmd(phydev, MDIO_MMD_PCS, + MDIO_MMD_PCS_MV_LED_FUNC_CTRL, + MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK, + FIELD_PREP(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK, + mode)); +} + +static int mv88q2xxx_led_hw_control_get(struct phy_device *phydev, u8 index, + unsigned long *rules) +{ + int val; + + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_LED_FUNC_CTRL); + if (val < 0) + return val; + + if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE) + val = FIELD_GET(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK, val); + else + val = FIELD_GET(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK, val); + + switch (val) { + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK: + *rules = BIT(TRIGGER_NETDEV_LINK); + break; + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1_ON: + *rules = BIT(TRIGGER_NETDEV_LINK_1000); + break; + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX: + *rules = BIT(TRIGGER_NETDEV_TX); + break; + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX: + *rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX); + break; + case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX: + *rules = BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | + BIT(TRIGGER_NETDEV_RX); + break; + default: + *rules = 0; + break; + } + + return 0; +} + static struct phy_driver mv88q2xxx_driver[] = { { .phy_id = MARVELL_PHY_ID_88Q2110, @@ -953,6 +1125,9 @@ static struct phy_driver mv88q2xxx_driver[] = { .get_sqi_max = mv88q2xxx_get_sqi_max, .suspend = mv88q2xxx_suspend, .resume = mv88q2xxx_resume, + .led_hw_is_supported = mv88q2xxx_led_hw_is_supported, + .led_hw_control_set = mv88q2xxx_led_hw_control_set, + .led_hw_control_get = mv88q2xxx_led_hw_control_get, }, };
Marvell 88Q2XXX devices support up to two configurable Light Emitting Diode (LED). Add minimal LED controller driver supporting the most common uses with the 'netdev' trigger. Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> --- Changes in v2: - Renamed MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK to MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_1_MASK (Stefan) - Renamed MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK to MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LED_0_MASK (Stefan) - Added comment for disabling tx disable feature (Stefan) - Added defines for all led functions (Andrew) - Move enabling of led0 function from mv88q2xxx_probe to mv88q222x_config_init. When the hardware reset pin is connected to a GPIO for example and we bring the interface down and up again, the content of the register MDIO_MMD_PCS_MV_RESET_CTRL is resetted to default. This means LED function is disabled and can't be enabled again. - Link to v1: https://lore.kernel.org/r/20250110-marvell-88q2xxx-leds-v1-1-22e7734941c2@gmail.com --- drivers/net/phy/marvell-88q2xxx.c | 175 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 175 insertions(+) --- base-commit: f84db3bc8abc2141839cdb9454061633a4ce1db7 change-id: 20241221-marvell-88q2xxx-leds-69a4037b5157 Best regards,