* [PATCH net-next v2] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx
@ 2025-02-07 16:24 Dimitri Fedrau
2025-02-08 16:44 ` Stefan Eichenberger
0 siblings, 1 reply; 5+ messages in thread
From: Dimitri Fedrau @ 2025-02-07 16:24 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Niklas Söderlund,
Gregor Herburger, Stefan Eichenberger
Cc: netdev, linux-kernel, Dimitri Fedrau
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(+)
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>
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net-next v2] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx
2025-02-07 16:24 [PATCH net-next v2] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx Dimitri Fedrau
@ 2025-02-08 16:44 ` Stefan Eichenberger
2025-02-09 8:41 ` Dimitri Fedrau
0 siblings, 1 reply; 5+ messages in thread
From: Stefan Eichenberger @ 2025-02-08 16:44 UTC (permalink / raw)
To: Dimitri Fedrau
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Niklas Söderlund,
Gregor Herburger, netdev, linux-kernel
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>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next v2] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx
2025-02-08 16:44 ` Stefan Eichenberger
@ 2025-02-09 8:41 ` Dimitri Fedrau
2025-02-10 0:27 ` Andrew Lunn
2025-02-10 7:22 ` Stefan Eichenberger
0 siblings, 2 replies; 5+ messages in thread
From: Dimitri Fedrau @ 2025-02-09 8:41 UTC (permalink / raw)
To: Stefan Eichenberger
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Niklas Söderlund,
Gregor Herburger, netdev, linux-kernel
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>
> >
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next v2] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx
2025-02-09 8:41 ` Dimitri Fedrau
@ 2025-02-10 0:27 ` Andrew Lunn
2025-02-10 7:22 ` Stefan Eichenberger
1 sibling, 0 replies; 5+ messages in thread
From: Andrew Lunn @ 2025-02-10 0:27 UTC (permalink / raw)
To: Dimitri Fedrau
Cc: Stefan Eichenberger, Heiner Kallweit, Russell King,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Niklas Söderlund, Gregor Herburger, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next v2] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx
2025-02-09 8:41 ` Dimitri Fedrau
2025-02-10 0:27 ` Andrew Lunn
@ 2025-02-10 7:22 ` Stefan Eichenberger
1 sibling, 0 replies; 5+ messages in thread
From: Stefan Eichenberger @ 2025-02-10 7:22 UTC (permalink / raw)
To: Dimitri Fedrau
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Niklas Söderlund,
Gregor Herburger, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-02-10 7:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-07 16:24 [PATCH net-next v2] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx Dimitri Fedrau
2025-02-08 16:44 ` Stefan Eichenberger
2025-02-09 8:41 ` Dimitri Fedrau
2025-02-10 0:27 ` Andrew Lunn
2025-02-10 7:22 ` Stefan Eichenberger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).