This is a continue of [1]. It was decided to take a more gradual
approach to implement LEDs support for switch and phy starting with
basic support and then implementing the hw control part when we have all
the prereq done.
This series implements only the brightness_set() and blink_set() ops.
An example of switch implementation is done with qca8k.
For PHY a more generic approach is used with implementing the LED
support in PHY core and with the user (in this case marvell) adding all
the required functions.
Currently we set the default-state as "keep" to not change the default
configuration of the declared LEDs since almost every switch have a
default configuration.
[1] https://lore.kernel.org/lkml/[email protected]/
Changes in new series v4:
- Changes in Andrew patch:
- net: phy: Add a binding for PHY LEDs:
- Rename phy_led: led_list to list
- Rename phy_device: led_list to leds
- Remove phy_leds_remove() since devm_ should do what is needed
- Fixup documentation for struct phy_led
- Fail probe on LED errors
- net: phy: phy_device: Call into the PHY driver to set LED brightness
- Moved phy_led::phydev from previous patch to here since it is first
used here.
- net: phy: marvell: Implement led_blink_set()
- Use int instead of unsigned
- net: phy: marvell: Add software control of the LEDs
- Use int instead of unsigned
- Add depends on LED_CLASS for qca8k Kconfig
- Fix Makefile for qca8k as suggested
- Move qca8k_setup_led_ctrl to separate header
- Move Documentation from dsa-port to ethernet-controller
- Drop trailing . from Andrew patch fro consistency
Changes in new series v3:
- Move QCA8K_LEDS Kconfig option from tristate to bool
- Use new helper led_init_default_state_get for default-state in qca8k
- Drop cled_qca8k_brightness_get() as there isn't a good way to describe
the mode the led is currently in
- Rework qca8k_led_brightness_get() to return true only when LED is set
to always ON
Changes in new series v2:
- Add LEDs node for rb3011
- Fix rb3011 switch node unevaluated properties while running
make dtbs_check
- Fix a copypaste error in qca8k-leds.c for port 4 required shift
- Drop phy-handle usage for qca8k and use qca8k_port_to_phy()
- Add review tag from Andrew
- Add Christian Marangi SOB in each Andrew patch
- Add extra description for dsa-port stressing that PHY have no access
and LED are controlled by the related MAC
- Add missing additionalProperties for dsa-port.yaml and ethernet-phy.yaml
Changes from the old v8 series:
- Drop linux,default-trigger set to netdev.
- Dropped every hw control related patch and implement only
blink_set and brightness_set
- Add default-state to "keep" for each LED node example
Andrew Lunn (6):
net: phy: Add a binding for PHY LEDs
net: phy: phy_device: Call into the PHY driver to set LED brightness
net: phy: marvell: Add software control of the LEDs
net: phy: phy_device: Call into the PHY driver to set LED blinking
net: phy: marvell: Implement led_blink_set()
arm: mvebu: dt: Add PHY LED support for 370-rd WAN port
Christian Marangi (8):
net: dsa: qca8k: move qca8k_port_to_phy() to header
net: dsa: qca8k: add LEDs basic support
net: dsa: qca8k: add LEDs blink_set() support
dt-bindings: net: ethernet-controller: Document support for LEDs node
dt-bindings: net: dsa: qca8k: add LEDs definition example
arm: qcom: dt: Drop unevaluated properties in switch nodes for rb3011
arm: qcom: dt: Add Switch LED for each port for rb3011
dt-bindings: net: phy: Document support for LEDs node
.../devicetree/bindings/net/dsa/qca8k.yaml | 24 ++
.../bindings/net/ethernet-controller.yaml | 21 ++
.../devicetree/bindings/net/ethernet-phy.yaml | 31 +++
arch/arm/boot/dts/armada-370-rd.dts | 14 ++
arch/arm/boot/dts/qcom-ipq8064-rb3011.dts | 124 +++++++++-
drivers/net/dsa/qca/Kconfig | 8 +
drivers/net/dsa/qca/Makefile | 3 +
drivers/net/dsa/qca/qca8k-8xxx.c | 20 +-
drivers/net/dsa/qca/qca8k-leds.c | 230 ++++++++++++++++++
drivers/net/dsa/qca/qca8k.h | 73 ++++++
drivers/net/dsa/qca/qca8k_leds.h | 16 ++
drivers/net/phy/Kconfig | 1 +
drivers/net/phy/marvell.c | 81 +++++-
drivers/net/phy/phy_device.c | 102 ++++++++
include/linux/phy.h | 35 +++
15 files changed, 759 insertions(+), 24 deletions(-)
create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
create mode 100644 drivers/net/dsa/qca/qca8k_leds.h
--
2.39.2
Add LEDs basic support for qca8k Switch Family by adding basic
brightness_set() support.
Since these LEDs refelect port status, the default label is set to
":port". DT binding should describe the color, function and number of
the leds using standard LEDs api.
These LEDs supports only blocking variant of the brightness_set()
function since they can sleep during access of the switch leds to set
the brightness.
While at it add to the qca8k header file each mode defined by the Switch
Documentation for future use.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/dsa/qca/Kconfig | 8 ++
drivers/net/dsa/qca/Makefile | 3 +
drivers/net/dsa/qca/qca8k-8xxx.c | 5 +
drivers/net/dsa/qca/qca8k-leds.c | 192 +++++++++++++++++++++++++++++++
drivers/net/dsa/qca/qca8k.h | 59 ++++++++++
drivers/net/dsa/qca/qca8k_leds.h | 16 +++
6 files changed, 283 insertions(+)
create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
create mode 100644 drivers/net/dsa/qca/qca8k_leds.h
diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
index ba339747362c..7a86d6d6a246 100644
--- a/drivers/net/dsa/qca/Kconfig
+++ b/drivers/net/dsa/qca/Kconfig
@@ -15,3 +15,11 @@ config NET_DSA_QCA8K
help
This enables support for the Qualcomm Atheros QCA8K Ethernet
switch chips.
+
+config NET_DSA_QCA8K_LEDS_SUPPORT
+ bool "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
+ depends on NET_DSA_QCA8K
+ depends on LEDS_CLASS
+ help
+ This enabled support for LEDs present on the Qualcomm Atheros
+ QCA8K Ethernet switch chips.
diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
index 701f1d199e93..ce66b1984e5f 100644
--- a/drivers/net/dsa/qca/Makefile
+++ b/drivers/net/dsa/qca/Makefile
@@ -2,3 +2,6 @@
obj-$(CONFIG_NET_DSA_AR9331) += ar9331.o
obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
qca8k-y += qca8k-common.o qca8k-8xxx.o
+ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
+qca8k-y += qca8k-leds.o
+endif
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 8dfc5db84700..5decf6fe3832 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -22,6 +22,7 @@
#include <linux/dsa/tag_qca.h>
#include "qca8k.h"
+#include "qca8k_leds.h"
static void
qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
@@ -1727,6 +1728,10 @@ qca8k_setup(struct dsa_switch *ds)
if (ret)
return ret;
+ ret = qca8k_setup_led_ctrl(priv);
+ if (ret)
+ return ret;
+
qca8k_setup_pcs(priv, &priv->pcs_port_0, 0);
qca8k_setup_pcs(priv, &priv->pcs_port_6, 6);
diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
new file mode 100644
index 000000000000..adbe7f6e2994
--- /dev/null
+++ b/drivers/net/dsa/qca/qca8k-leds.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/regmap.h>
+#include <net/dsa.h>
+
+#include "qca8k.h"
+#include "qca8k_leds.h"
+
+static int
+qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+ switch (port_num) {
+ case 0:
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+ reg_info->shift = QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT;
+ break;
+ case 1:
+ case 2:
+ case 3:
+ /* Port 123 are controlled on a different reg */
+ reg_info->reg = QCA8K_LED_CTRL_REG(3);
+ reg_info->shift = QCA8K_LED_PHY123_PATTERN_EN_SHIFT(port_num, led_num);
+ break;
+ case 4:
+ reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+ reg_info->shift = QCA8K_LED_PHY4_CONTROL_RULE_SHIFT;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int
+qca8k_led_brightness_set(struct qca8k_led *led,
+ enum led_brightness brightness)
+{
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 mask, val = QCA8K_LED_ALWAYS_OFF;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ if (brightness)
+ val = QCA8K_LED_ALWAYS_ON;
+
+ if (led->port_num == 0 || led->port_num == 4) {
+ mask = QCA8K_LED_PATTERN_EN_MASK;
+ val <<= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ return regmap_update_bits(priv->regmap, reg_info.reg,
+ mask << reg_info.shift,
+ val << reg_info.shift);
+}
+
+static int
+qca8k_cled_brightness_set_blocking(struct led_classdev *ldev,
+ enum led_brightness brightness)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+ return qca8k_led_brightness_set(led, brightness);
+}
+
+static enum led_brightness
+qca8k_led_brightness_get(struct qca8k_led *led)
+{
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+ u32 val;
+ int ret;
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ ret = regmap_read(priv->regmap, reg_info.reg, &val);
+ if (ret)
+ return 0;
+
+ val >>= reg_info.shift;
+
+ if (led->port_num == 0 || led->port_num == 4) {
+ val &= QCA8K_LED_PATTERN_EN_MASK;
+ val >>= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ /* Assume brightness ON only when the LED is set to always ON */
+ return val == QCA8K_LED_ALWAYS_ON;
+}
+
+static int
+qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
+{
+ struct fwnode_handle *led = NULL, *leds = NULL;
+ struct led_init_data init_data = { };
+ enum led_default_state state;
+ struct qca8k_led *port_led;
+ int led_num, port_index;
+ int ret;
+
+ leds = fwnode_get_named_child_node(port, "leds");
+ if (!leds) {
+ dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
+ port_num);
+ return 0;
+ }
+
+ fwnode_for_each_child_node(leds, led) {
+ /* Reg represent the led number of the port.
+ * Each port can have at least 3 leds attached
+ * Commonly:
+ * 1. is gigabit led
+ * 2. is mbit led
+ * 3. additional status led
+ */
+ if (fwnode_property_read_u32(led, "reg", &led_num))
+ continue;
+
+ if (led_num >= QCA8K_LED_PORT_COUNT) {
+ dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
+ continue;
+ }
+
+ port_index = 3 * port_num + led_num;
+
+ port_led = &priv->ports_led[port_index];
+ port_led->port_num = port_num;
+ port_led->led_num = led_num;
+ port_led->priv = priv;
+
+ state = led_init_default_state_get(led);
+ switch (state) {
+ case LEDS_DEFSTATE_ON:
+ port_led->cdev.brightness = 1;
+ qca8k_led_brightness_set(port_led, 1);
+ break;
+ case LEDS_DEFSTATE_KEEP:
+ port_led->cdev.brightness =
+ qca8k_led_brightness_get(port_led);
+ break;
+ default:
+ port_led->cdev.brightness = 0;
+ qca8k_led_brightness_set(port_led, 0);
+ }
+
+ port_led->cdev.max_brightness = 1;
+ port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
+ init_data.default_label = ":port";
+ init_data.devicename = "qca8k";
+ init_data.fwnode = led;
+
+ ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
+ if (ret)
+ dev_warn(priv->dev, "Failed to int led");
+ }
+
+ return 0;
+}
+
+int
+qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+ struct fwnode_handle *ports, *port;
+ int port_num;
+ int ret;
+
+ ports = device_get_named_child_node(priv->dev, "ports");
+ if (!ports) {
+ dev_info(priv->dev, "No ports node specified in device tree!\n");
+ return 0;
+ }
+
+ fwnode_for_each_child_node(ports, port) {
+ if (fwnode_property_read_u32(port, "reg", &port_num))
+ continue;
+
+ /* Each port can have at least 3 different leds attached.
+ * Switch port starts from 0 to 6, but port 0 and 6 are CPU
+ * port. The port index needs to be decreased by one to identify
+ * the correct port for LED setup.
+ */
+ ret = qca8k_parse_port_leds(priv, port, qca8k_port_to_phy(port_num));
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 4e48e4dd8b0f..3c3c072fa9c2 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -11,6 +11,7 @@
#include <linux/delay.h>
#include <linux/regmap.h>
#include <linux/gpio.h>
+#include <linux/leds.h>
#include <linux/dsa/tag_qca.h>
#define QCA8K_ETHERNET_MDIO_PRIORITY 7
@@ -85,6 +86,50 @@
#define QCA8K_MDIO_MASTER_DATA(x) FIELD_PREP(QCA8K_MDIO_MASTER_DATA_MASK, x)
#define QCA8K_MDIO_MASTER_MAX_PORTS 5
#define QCA8K_MDIO_MASTER_MAX_REG 32
+
+/* LED control register */
+#define QCA8K_LED_COUNT 15
+#define QCA8K_LED_PORT_COUNT 3
+#define QCA8K_LED_RULE_COUNT 6
+#define QCA8K_LED_RULE_MAX 11
+
+#define QCA8K_LED_PHY123_PATTERN_EN_SHIFT(_phy, _led) ((((_phy) - 1) * 6) + 8 + (2 * (_led)))
+#define QCA8K_LED_PHY123_PATTERN_EN_MASK GENMASK(1, 0)
+
+#define QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT 0
+#define QCA8K_LED_PHY4_CONTROL_RULE_SHIFT 16
+
+#define QCA8K_LED_CTRL_REG(_i) (0x050 + (_i) * 4)
+#define QCA8K_LED_CTRL0_REG 0x50
+#define QCA8K_LED_CTRL1_REG 0x54
+#define QCA8K_LED_CTRL2_REG 0x58
+#define QCA8K_LED_CTRL3_REG 0x5C
+#define QCA8K_LED_CTRL_SHIFT(_i) (((_i) % 2) * 16)
+#define QCA8K_LED_CTRL_MASK GENMASK(15, 0)
+#define QCA8K_LED_RULE_MASK GENMASK(13, 0)
+#define QCA8K_LED_BLINK_FREQ_MASK GENMASK(1, 0)
+#define QCA8K_LED_BLINK_FREQ_SHITF 0
+#define QCA8K_LED_BLINK_2HZ 0
+#define QCA8K_LED_BLINK_4HZ 1
+#define QCA8K_LED_BLINK_8HZ 2
+#define QCA8K_LED_BLINK_AUTO 3
+#define QCA8K_LED_LINKUP_OVER_MASK BIT(2)
+#define QCA8K_LED_TX_BLINK_MASK BIT(4)
+#define QCA8K_LED_RX_BLINK_MASK BIT(5)
+#define QCA8K_LED_COL_BLINK_MASK BIT(7)
+#define QCA8K_LED_LINK_10M_EN_MASK BIT(8)
+#define QCA8K_LED_LINK_100M_EN_MASK BIT(9)
+#define QCA8K_LED_LINK_1000M_EN_MASK BIT(10)
+#define QCA8K_LED_POWER_ON_LIGHT_MASK BIT(11)
+#define QCA8K_LED_HALF_DUPLEX_MASK BIT(12)
+#define QCA8K_LED_FULL_DUPLEX_MASK BIT(13)
+#define QCA8K_LED_PATTERN_EN_MASK GENMASK(15, 14)
+#define QCA8K_LED_PATTERN_EN_SHIFT 14
+#define QCA8K_LED_ALWAYS_OFF 0
+#define QCA8K_LED_ALWAYS_BLINK_4HZ 1
+#define QCA8K_LED_ALWAYS_ON 2
+#define QCA8K_LED_RULE_CONTROLLED 3
+
#define QCA8K_GOL_MAC_ADDR0 0x60
#define QCA8K_GOL_MAC_ADDR1 0x64
#define QCA8K_MAX_FRAME_SIZE 0x78
@@ -383,6 +428,19 @@ struct qca8k_pcs {
int port;
};
+struct qca8k_led_pattern_en {
+ u32 reg;
+ u8 shift;
+};
+
+struct qca8k_led {
+ u8 port_num;
+ u8 led_num;
+ u16 old_rule;
+ struct qca8k_priv *priv;
+ struct led_classdev cdev;
+};
+
struct qca8k_priv {
u8 switch_id;
u8 switch_revision;
@@ -407,6 +465,7 @@ struct qca8k_priv {
struct qca8k_pcs pcs_port_0;
struct qca8k_pcs pcs_port_6;
const struct qca8k_match_data *info;
+ struct qca8k_led ports_led[QCA8K_LED_COUNT];
};
struct qca8k_mib_desc {
diff --git a/drivers/net/dsa/qca/qca8k_leds.h b/drivers/net/dsa/qca/qca8k_leds.h
new file mode 100644
index 000000000000..ab367f05b173
--- /dev/null
+++ b/drivers/net/dsa/qca/qca8k_leds.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __QCA8K_LEDS_H
+#define __QCA8K_LEDS_H
+
+/* Leds Support function */
+#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
+int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
+#else
+static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+ return 0;
+}
+#endif
+
+#endif /* __QCA8K_LEDS_H */
--
2.39.2
Add LEDs blink_set() support to qca8k Switch Family.
These LEDs support hw accellerated blinking at a fixed rate
of 4Hz.
Reject any other value since not supported by the LEDs switch.
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
drivers/net/dsa/qca/qca8k-leds.c | 38 ++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
index adbe7f6e2994..c229575c7e8c 100644
--- a/drivers/net/dsa/qca/qca8k-leds.c
+++ b/drivers/net/dsa/qca/qca8k-leds.c
@@ -92,6 +92,43 @@ qca8k_led_brightness_get(struct qca8k_led *led)
return val == QCA8K_LED_ALWAYS_ON;
}
+static int
+qca8k_cled_blink_set(struct led_classdev *ldev,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+ u32 mask, val = QCA8K_LED_ALWAYS_BLINK_4HZ;
+ struct qca8k_led_pattern_en reg_info;
+ struct qca8k_priv *priv = led->priv;
+
+ if (*delay_on == 0 && *delay_off == 0) {
+ *delay_on = 125;
+ *delay_off = 125;
+ }
+
+ if (*delay_on != 125 || *delay_off != 125) {
+ /* The hardware only supports blinking at 4Hz. Fall back
+ * to software implementation in other cases.
+ */
+ return -EINVAL;
+ }
+
+ qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
+
+ if (led->port_num == 0 || led->port_num == 4) {
+ mask = QCA8K_LED_PATTERN_EN_MASK;
+ val <<= QCA8K_LED_PATTERN_EN_SHIFT;
+ } else {
+ mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
+ }
+
+ regmap_update_bits(priv->regmap, reg_info.reg, mask << reg_info.shift,
+ val << reg_info.shift);
+
+ return 0;
+}
+
static int
qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
{
@@ -149,6 +186,7 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p
port_led->cdev.max_brightness = 1;
port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
+ port_led->cdev.blink_set = qca8k_cled_blink_set;
init_data.default_label = ":port";
init_data.devicename = "qca8k";
init_data.fwnode = led;
--
2.39.2
Move qca8k_port_to_phy() to qca8k header as it's useful for future
reference in Switch LEDs module since the same logic is applied to get
the right index of the switch port.
Make it inline as it's simple function that just decrease the port.
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
drivers/net/dsa/qca/qca8k-8xxx.c | 15 ---------------
drivers/net/dsa/qca/qca8k.h | 14 ++++++++++++++
2 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 2f224b166bbb..8dfc5db84700 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -716,21 +716,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
return ret;
}
-static u32
-qca8k_port_to_phy(int port)
-{
- /* From Andrew Lunn:
- * Port 0 has no internal phy.
- * Port 1 has an internal PHY at MDIO address 0.
- * Port 2 has an internal PHY at MDIO address 1.
- * ...
- * Port 5 has an internal PHY at MDIO address 4.
- * Port 6 has no internal PHY.
- */
-
- return port - 1;
-}
-
static int
qca8k_mdio_busy_wait(struct mii_bus *bus, u32 reg, u32 mask)
{
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 03514f7a20be..4e48e4dd8b0f 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -422,6 +422,20 @@ struct qca8k_fdb {
u8 mac[6];
};
+static inline u32 qca8k_port_to_phy(int port)
+{
+ /* From Andrew Lunn:
+ * Port 0 has no internal phy.
+ * Port 1 has an internal PHY at MDIO address 0.
+ * Port 2 has an internal PHY at MDIO address 1.
+ * ...
+ * Port 5 has an internal PHY at MDIO address 4.
+ * Port 6 has no internal PHY.
+ */
+
+ return port - 1;
+}
+
/* Common setup function */
extern const struct qca8k_mib_desc ar8327_mib[];
extern const struct regmap_access_table qca8k_readable_table;
--
2.39.2
From: Andrew Lunn <[email protected]>
Define common binding parsing for all PHY drivers with LEDs using
phylib. Parse the DT as part of the phy_probe and add LEDs to the
linux LED class infrastructure. For the moment, provide a dummy
brightness function, which will later be replaced with a call into the
PHY driver.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/Kconfig | 1 +
drivers/net/phy/phy_device.c | 75 ++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 16 ++++++++
3 files changed, 92 insertions(+)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index f5df2edc94a5..666efa6b1c8e 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -16,6 +16,7 @@ config PHYLINK
menuconfig PHYLIB
tristate "PHY Device support and infrastructure"
depends on NETDEVICES
+ depends on LEDS_CLASS
select MDIO_DEVICE
select MDIO_DEVRES
help
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9ba8f973f26f..ee800f93c8c3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -19,10 +19,12 @@
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/list.h>
#include <linux/mdio.h>
#include <linux/mii.h>
#include <linux/mm.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/netdevice.h>
#include <linux/phy.h>
#include <linux/phy_led_triggers.h>
@@ -658,6 +660,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
device_initialize(&mdiodev->dev);
dev->state = PHY_DOWN;
+ INIT_LIST_HEAD(&dev->leds);
mutex_init(&dev->lock);
INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
@@ -2964,6 +2967,73 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
return phydrv->config_intr && phydrv->handle_interrupt;
}
+/* Dummy implementation until calls into PHY driver are added */
+static int phy_led_set_brightness(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ return 0;
+}
+
+static int of_phy_led(struct phy_device *phydev,
+ struct device_node *led)
+{
+ struct device *dev = &phydev->mdio.dev;
+ struct led_init_data init_data = {};
+ struct led_classdev *cdev;
+ struct phy_led *phyled;
+ int err;
+
+ phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
+ if (!phyled)
+ return -ENOMEM;
+
+ cdev = &phyled->led_cdev;
+
+ err = of_property_read_u32(led, "reg", &phyled->index);
+ if (err)
+ return err;
+
+ cdev->brightness_set_blocking = phy_led_set_brightness;
+ cdev->max_brightness = 1;
+ init_data.devicename = dev_name(&phydev->mdio.dev);
+ init_data.fwnode = of_fwnode_handle(led);
+
+ err = devm_led_classdev_register_ext(dev, cdev, &init_data);
+ if (err)
+ return err;
+
+ list_add(&phyled->list, &phydev->leds);
+
+ return 0;
+}
+
+static int of_phy_leds(struct phy_device *phydev)
+{
+ struct device_node *node = phydev->mdio.dev.of_node;
+ struct device_node *leds, *led;
+ int err;
+
+ if (!IS_ENABLED(CONFIG_OF_MDIO))
+ return 0;
+
+ if (!node)
+ return 0;
+
+ leds = of_get_child_by_name(node, "leds");
+ if (!leds)
+ return 0;
+
+ for_each_available_child_of_node(leds, led) {
+ err = of_phy_led(phydev, led);
+ if (err) {
+ of_node_put(led);
+ return err;
+ }
+ }
+
+ return 0;
+}
+
/**
* fwnode_mdio_find_device - Given a fwnode, find the mdio_device
* @fwnode: pointer to the mdio_device's fwnode
@@ -3142,6 +3212,11 @@ static int phy_probe(struct device *dev)
/* Set the state to READY by default */
phydev->state = PHY_READY;
+ /* Get the LEDs from the device tree, and instantiate standard
+ * LEDs for them.
+ */
+ err = of_phy_leds(phydev);
+
out:
/* Assert the reset signal */
if (err)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fbeba4fee8d4..88a77ff60be9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -14,6 +14,7 @@
#include <linux/compiler.h>
#include <linux/spinlock.h>
#include <linux/ethtool.h>
+#include <linux/leds.h>
#include <linux/linkmode.h>
#include <linux/netlink.h>
#include <linux/mdio.h>
@@ -595,6 +596,7 @@ struct macsec_ops;
* @phy_num_led_triggers: Number of triggers in @phy_led_triggers
* @led_link_trigger: LED trigger for link up/down
* @last_triggered: last LED trigger for link speed
+ * @leds: list of PHY LED structures
* @master_slave_set: User requested master/slave configuration
* @master_slave_get: Current master/slave advertisement
* @master_slave_state: Current master/slave configuration
@@ -690,6 +692,7 @@ struct phy_device {
struct phy_led_trigger *led_link_trigger;
#endif
+ struct list_head leds;
/*
* Interrupt number for this PHY
@@ -825,6 +828,19 @@ struct phy_plca_status {
bool pst;
};
+/**
+ * struct phy_led: An LED driven by the PHY
+ *
+ * @list: List of LEDs
+ * @led_cdev: Standard LED class structure
+ * @index: Number of the LED
+ */
+struct phy_led {
+ struct list_head list;
+ struct led_classdev led_cdev;
+ u32 index;
+};
+
/**
* struct phy_driver - Driver structure for a particular PHY type
*
--
2.39.2
From: Andrew Lunn <[email protected]>
Linux LEDs can be requested to perform hardware accelerated
blinking. Pass this to the PHY driver, if it implements the op.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/phy_device.c | 18 ++++++++++++++++++
include/linux/phy.h | 8 ++++++++
2 files changed, 26 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c7312a9e820d..b4be02a21bb8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2981,6 +2981,22 @@ static int phy_led_set_brightness(struct led_classdev *led_cdev,
return err;
}
+static int phy_led_blink_set(struct led_classdev *led_cdev,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ struct phy_led *phyled = to_phy_led(led_cdev);
+ struct phy_device *phydev = phyled->phydev;
+ int err;
+
+ mutex_lock(&phydev->lock);
+ err = phydev->drv->led_blink_set(phydev, phyled->index,
+ delay_on, delay_off);
+ mutex_unlock(&phydev->lock);
+
+ return err;
+}
+
static int of_phy_led(struct phy_device *phydev,
struct device_node *led)
{
@@ -3003,6 +3019,8 @@ static int of_phy_led(struct phy_device *phydev,
if (phydev->drv->led_brightness_set)
cdev->brightness_set_blocking = phy_led_set_brightness;
+ if (phydev->drv->led_blink_set)
+ cdev->blink_set = phy_led_blink_set;
cdev->max_brightness = 1;
init_data.devicename = dev_name(&phydev->mdio.dev);
init_data.fwnode = of_fwnode_handle(led);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 94fd21d5e145..58cb44960573 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1074,6 +1074,14 @@ struct phy_driver {
*/
int (*led_brightness_set)(struct phy_device *dev,
u32 index, enum led_brightness value);
+ /* Activate hardware accelerated blink, delays are in milliseconds
+ * and if both are zero then a sensible default should be chosen.
+ * The call should adjust the timings in that case and if it can't
+ * match the values specified exactly.
+ */
+ int (*led_blink_set)(struct phy_device *dev, u32 index,
+ unsigned long *delay_on,
+ unsigned long *delay_off);
};
#define to_phy_driver(d) container_of(to_mdio_common_driver(d), \
struct phy_driver, mdiodrv)
--
2.39.2
From: Andrew Lunn <[email protected]>
Add a brightness function, so the LEDs can be controlled from
software using the standard Linux LED infrastructure.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/marvell.c | 45 ++++++++++++++++++++++++++++++++++-----
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 0d706ee266af..cadf9da13b82 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -144,11 +144,13 @@
/* WOL Event Interrupt Enable */
#define MII_88E1318S_PHY_CSIER_WOL_EIE BIT(7)
-/* LED Timer Control Register */
-#define MII_88E1318S_PHY_LED_TCR 0x12
-#define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
-#define MII_88E1318S_PHY_LED_TCR_INTn_ENABLE BIT(7)
-#define MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW BIT(11)
+#define MII_88E1318S_PHY_LED_FUNC 0x10
+#define MII_88E1318S_PHY_LED_FUNC_OFF (0x8)
+#define MII_88E1318S_PHY_LED_FUNC_ON (0x9)
+#define MII_88E1318S_PHY_LED_TCR 0x12
+#define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
+#define MII_88E1318S_PHY_LED_TCR_INTn_ENABLE BIT(7)
+#define MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW BIT(11)
/* Magic Packet MAC address registers */
#define MII_88E1318S_PHY_MAGIC_PACKET_WORD2 0x17
@@ -2832,6 +2834,34 @@ static int marvell_hwmon_probe(struct phy_device *phydev)
}
#endif
+static int m88e1318_led_brightness_set(struct phy_device *phydev,
+ u32 index, enum led_brightness value)
+{
+ int reg;
+
+ reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE,
+ MII_88E1318S_PHY_LED_FUNC);
+ if (reg < 0)
+ return reg;
+
+ switch (index) {
+ case 0:
+ case 1:
+ case 2:
+ reg &= ~(0xf << (4 * index));
+ if (value == LED_OFF)
+ reg |= MII_88E1318S_PHY_LED_FUNC_OFF << (4 * index);
+ else
+ reg |= MII_88E1318S_PHY_LED_FUNC_ON << (4 * index);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return phy_write_paged(phydev, MII_MARVELL_LED_PAGE,
+ MII_88E1318S_PHY_LED_FUNC, reg);
+}
+
static int marvell_probe(struct phy_device *phydev)
{
struct marvell_priv *priv;
@@ -3081,6 +3111,7 @@ static struct phy_driver marvell_drivers[] = {
.get_sset_count = marvell_get_sset_count,
.get_strings = marvell_get_strings,
.get_stats = marvell_get_stats,
+ .led_brightness_set = m88e1318_led_brightness_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1145,
@@ -3187,6 +3218,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_start = marvell_vct7_cable_test_start,
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
+ .led_brightness_set = m88e1318_led_brightness_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1540,
@@ -3213,6 +3245,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_start = marvell_vct7_cable_test_start,
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
+ .led_brightness_set = m88e1318_led_brightness_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1545,
@@ -3239,6 +3272,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_start = marvell_vct7_cable_test_start,
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
+ .led_brightness_set = m88e1318_led_brightness_set,
},
{
.phy_id = MARVELL_PHY_ID_88E3016,
@@ -3380,6 +3414,7 @@ static struct phy_driver marvell_drivers[] = {
.get_stats = marvell_get_stats,
.get_tunable = m88e1540_get_tunable,
.set_tunable = m88e1540_set_tunable,
+ .led_brightness_set = m88e1318_led_brightness_set,
},
};
--
2.39.2
From: Andrew Lunn <[email protected]>
Linux LEDs can be software controlled via the brightness file in /sys.
LED drivers need to implement a brightness_set function which the core
will call. Implement an intermediary in phy_device, which will call
into the phy driver if it implements the necessary function.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/phy_device.c | 15 ++++++++++++---
include/linux/phy.h | 11 +++++++++++
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ee800f93c8c3..c7312a9e820d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2967,11 +2967,18 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
return phydrv->config_intr && phydrv->handle_interrupt;
}
-/* Dummy implementation until calls into PHY driver are added */
static int phy_led_set_brightness(struct led_classdev *led_cdev,
enum led_brightness value)
{
- return 0;
+ struct phy_led *phyled = to_phy_led(led_cdev);
+ struct phy_device *phydev = phyled->phydev;
+ int err;
+
+ mutex_lock(&phydev->lock);
+ err = phydev->drv->led_brightness_set(phydev, phyled->index, value);
+ mutex_unlock(&phydev->lock);
+
+ return err;
}
static int of_phy_led(struct phy_device *phydev,
@@ -2988,12 +2995,14 @@ static int of_phy_led(struct phy_device *phydev,
return -ENOMEM;
cdev = &phyled->led_cdev;
+ phyled->phydev = phydev;
err = of_property_read_u32(led, "reg", &phyled->index);
if (err)
return err;
- cdev->brightness_set_blocking = phy_led_set_brightness;
+ if (phydev->drv->led_brightness_set)
+ cdev->brightness_set_blocking = phy_led_set_brightness;
cdev->max_brightness = 1;
init_data.devicename = dev_name(&phydev->mdio.dev);
init_data.fwnode = of_fwnode_handle(led);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 88a77ff60be9..94fd21d5e145 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -832,15 +832,19 @@ struct phy_plca_status {
* struct phy_led: An LED driven by the PHY
*
* @list: List of LEDs
+ * @phydev: PHY this LED is attached to
* @led_cdev: Standard LED class structure
* @index: Number of the LED
*/
struct phy_led {
struct list_head list;
+ struct phy_device *phydev;
struct led_classdev led_cdev;
u32 index;
};
+#define to_phy_led(d) container_of(d, struct phy_led, led_cdev)
+
/**
* struct phy_driver - Driver structure for a particular PHY type
*
@@ -1063,6 +1067,13 @@ struct phy_driver {
/** @get_plca_status: Return the current PLCA status info */
int (*get_plca_status)(struct phy_device *dev,
struct phy_plca_status *plca_st);
+
+ /* Set a PHY LED brightness. Index indicates which of the PHYs
+ * led should be set. Value follows the standard LED class meaning,
+ * e.g. LED_OFF, LED_HALF, LED_FULL.
+ */
+ int (*led_brightness_set)(struct phy_device *dev,
+ u32 index, enum led_brightness value);
};
#define to_phy_driver(d) container_of(to_mdio_common_driver(d), \
struct phy_driver, mdiodrv)
--
2.39.2
Add LEDs definition example for qca8k Switch Family to describe how they
should be defined for a correct usage.
Signed-off-by: Christian Marangi <[email protected]>
---
.../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index 389892592aac..2e9c14af0223 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -18,6 +18,8 @@ description:
PHY it is connected to. In this config, an internal mdio-bus is registered and
the MDIO master is used for communication. Mixed external and internal
mdio-bus configurations are not supported by the hardware.
+ Each phy has at least 3 LEDs connected and can be declared
+ using the standard LEDs structure.
properties:
compatible:
@@ -117,6 +119,7 @@ unevaluatedProperties: false
examples:
- |
#include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/leds/common.h>
mdio {
#address-cells = <1>;
@@ -226,6 +229,27 @@ examples:
label = "lan1";
phy-mode = "internal";
phy-handle = <&internal_phy_port1>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_AMBER>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ default-state = "keep";
+ };
+ };
};
port@2 {
--
2.39.2
Document support for LEDs node in ethernet-controller.
Ethernet Controller may support different LEDs that can be configured
for different operation like blinking on traffic event or port link.
Also add some Documentation to describe the difference of these nodes
compared to PHY LEDs, since ethernet-controller LEDs are controllable
by the ethernet controller regs and the possible intergated PHY doesn't
have control on them.
Signed-off-by: Christian Marangi <[email protected]>
---
.../bindings/net/ethernet-controller.yaml | 21 +++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 00be387984ac..a93673592314 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -222,6 +222,27 @@ properties:
required:
- speed
+ leds:
+ type: object
+ description:
+ Describes the LEDs associated by Ethernet Controller.
+ These LEDs are not integrated in the PHY and PHY doesn't have any
+ control on them. Ethernet Controller regs are used to control
+ these defined LEDs.
+
+ properties:
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ patternProperties:
+ '^led(@[a-f0-9]+)?$':
+ $ref: /schemas/leds/common.yaml#
+
+ additionalProperties: false
+
dependencies:
pcs-handle-names: [pcs-handle]
--
2.39.2
From: Andrew Lunn <[email protected]>
The Marvell PHY can blink the LEDs, simple on/off. All LEDs blink at
the same rate, and the reset default is 84ms per blink, which is
around 12Hz.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/marvell.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index cadf9da13b82..c86283a4c0a9 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -147,6 +147,8 @@
#define MII_88E1318S_PHY_LED_FUNC 0x10
#define MII_88E1318S_PHY_LED_FUNC_OFF (0x8)
#define MII_88E1318S_PHY_LED_FUNC_ON (0x9)
+#define MII_88E1318S_PHY_LED_FUNC_HI_Z (0xa)
+#define MII_88E1318S_PHY_LED_FUNC_BLINK (0xb)
#define MII_88E1318S_PHY_LED_TCR 0x12
#define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
#define MII_88E1318S_PHY_LED_TCR_INTn_ENABLE BIT(7)
@@ -2862,6 +2864,35 @@ static int m88e1318_led_brightness_set(struct phy_device *phydev,
MII_88E1318S_PHY_LED_FUNC, reg);
}
+static int m88e1318_led_blink_set(struct phy_device *phydev, u32 index,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ int reg;
+
+ reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE,
+ MII_88E1318S_PHY_LED_FUNC);
+ if (reg < 0)
+ return reg;
+
+ switch (index) {
+ case 0:
+ case 1:
+ case 2:
+ reg &= ~(0xf << (4 * index));
+ reg |= MII_88E1318S_PHY_LED_FUNC_BLINK << (4 * index);
+ /* Reset default is 84ms */
+ *delay_on = 84 / 2;
+ *delay_off = 84 / 2;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return phy_write_paged(phydev, MII_MARVELL_LED_PAGE,
+ MII_88E1318S_PHY_LED_FUNC, reg);
+}
+
static int marvell_probe(struct phy_device *phydev)
{
struct marvell_priv *priv;
@@ -3112,6 +3143,7 @@ static struct phy_driver marvell_drivers[] = {
.get_strings = marvell_get_strings,
.get_stats = marvell_get_stats,
.led_brightness_set = m88e1318_led_brightness_set,
+ .led_blink_set = m88e1318_led_blink_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1145,
@@ -3219,6 +3251,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
.led_brightness_set = m88e1318_led_brightness_set,
+ .led_blink_set = m88e1318_led_blink_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1540,
@@ -3246,6 +3279,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
.led_brightness_set = m88e1318_led_brightness_set,
+ .led_blink_set = m88e1318_led_blink_set,
},
{
.phy_id = MARVELL_PHY_ID_88E1545,
@@ -3273,6 +3307,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
.led_brightness_set = m88e1318_led_brightness_set,
+ .led_blink_set = m88e1318_led_blink_set,
},
{
.phy_id = MARVELL_PHY_ID_88E3016,
@@ -3415,6 +3450,7 @@ static struct phy_driver marvell_drivers[] = {
.get_tunable = m88e1540_get_tunable,
.set_tunable = m88e1540_set_tunable,
.led_brightness_set = m88e1318_led_brightness_set,
+ .led_blink_set = m88e1318_led_blink_set,
},
};
--
2.39.2
IPQ8064 MikroTik RB3011UiAS-RM DT have currently unevaluted properties
in the 2 switch nodes. The bindings #address-cells and #size-cells are
redundant and cause warning for 'Unevaluated properties are not
allowed'.
Drop these bindings to mute these warning as they should not be there
from the start.
Cc: Jonathan McDowell <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
arch/arm/boot/dts/qcom-ipq8064-rb3011.dts | 4 ----
1 file changed, 4 deletions(-)
diff --git a/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts b/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
index f908889c4f95..47a5d1849c72 100644
--- a/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
+++ b/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
@@ -38,8 +38,6 @@ mdio0: mdio-0 {
switch0: switch@10 {
compatible = "qca,qca8337";
- #address-cells = <1>;
- #size-cells = <0>;
dsa,member = <0 0>;
@@ -105,8 +103,6 @@ mdio1: mdio-1 {
switch1: switch@14 {
compatible = "qca,qca8337";
- #address-cells = <1>;
- #size-cells = <0>;
dsa,member = <1 0>;
--
2.39.2
Add Switch LED for each port for MikroTik RB3011UiAS-RM.
MikroTik RB3011UiAS-RM is a 10 port device with 2 qca8337 switch chips
connected.
It was discovered that in the hardware design all 3 Switch LED trace of
the related port is connected to the same LED. This was discovered by
setting to 'always on' the related led in the switch regs and noticing
that all 3 LED for the specific port (for example for port 1) cause the
connected LED for port 1 to turn on. As an extra test we tried enabling
2 different LED for the port resulting in the LED turned off only if
every led in the reg was off.
Aside from this funny and strange hardware implementation, the device
itself have one green LED for each port, resulting in 10 green LED one
for each of the 10 supported port.
Cc: Jonathan McDowell <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
arch/arm/boot/dts/qcom-ipq8064-rb3011.dts | 120 ++++++++++++++++++++++
1 file changed, 120 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts b/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
index 47a5d1849c72..472b5a2912a1 100644
--- a/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
+++ b/arch/arm/boot/dts/qcom-ipq8064-rb3011.dts
@@ -65,26 +65,86 @@ fixed-link {
port@1 {
reg = <1>;
label = "sw1";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ };
+ };
};
port@2 {
reg = <2>;
label = "sw2";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <2>;
+ };
+ };
};
port@3 {
reg = <3>;
label = "sw3";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <3>;
+ };
+ };
};
port@4 {
reg = <4>;
label = "sw4";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <4>;
+ };
+ };
};
port@5 {
reg = <5>;
label = "sw5";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <5>;
+ };
+ };
};
};
};
@@ -130,26 +190,86 @@ fixed-link {
port@1 {
reg = <1>;
label = "sw6";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <6>;
+ };
+ };
};
port@2 {
reg = <2>;
label = "sw7";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <7>;
+ };
+ };
};
port@3 {
reg = <3>;
label = "sw8";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <8>;
+ };
+ };
};
port@4 {
reg = <4>;
label = "sw9";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <9>;
+ };
+ };
};
port@5 {
reg = <5>;
label = "sw10";
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <10>;
+ };
+ };
};
};
};
--
2.39.2
Document support for LEDs node in phy and add an example for it.
PHY LED will have to match led pattern and should be treated as a
generic led.
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
.../devicetree/bindings/net/ethernet-phy.yaml | 31 +++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 1327b81f15a2..84e15cee27c7 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -197,6 +197,22 @@ properties:
PHY's that have configurable TX internal delays. If this property is
present then the PHY applies the TX delay.
+ leds:
+ type: object
+
+ properties:
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ patternProperties:
+ '^led(@[a-f0-9]+)?$':
+ $ref: /schemas/leds/common.yaml#
+
+ additionalProperties: false
+
required:
- reg
@@ -204,6 +220,8 @@ additionalProperties: true
examples:
- |
+ #include <dt-bindings/leds/common.h>
+
ethernet {
#address-cells = <1>;
#size-cells = <0>;
@@ -219,5 +237,18 @@ examples:
reset-gpios = <&gpio1 4 1>;
reset-assert-us = <1000>;
reset-deassert-us = <2000>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ default-state = "keep";
+ };
+ };
};
};
--
2.39.2
From: Andrew Lunn <[email protected]>
The WAN port of the 370-RD has a Marvell PHY, with one LED on
the front panel. List this LED in the device tree.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Christian Marangi <[email protected]>
---
arch/arm/boot/dts/armada-370-rd.dts | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/arm/boot/dts/armada-370-rd.dts b/arch/arm/boot/dts/armada-370-rd.dts
index be005c9f42ef..15b36aa34ef4 100644
--- a/arch/arm/boot/dts/armada-370-rd.dts
+++ b/arch/arm/boot/dts/armada-370-rd.dts
@@ -20,6 +20,7 @@
/dts-v1/;
#include <dt-bindings/input/input.h>
#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/leds/common.h>
#include <dt-bindings/gpio/gpio.h>
#include "armada-370.dtsi"
@@ -135,6 +136,19 @@ &mdio {
pinctrl-names = "default";
phy0: ethernet-phy@0 {
reg = <0>;
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ label = "WAN";
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ function-enumerator = <1>;
+ linux,default-trigger = "netdev";
+ };
+ };
};
switch: switch@10 {
--
2.39.2
On Fri, 17 Mar 2023 03:31:15 +0100
Christian Marangi <[email protected]> wrote:
> + cdev->brightness_set_blocking = phy_led_set_brightness;
> + cdev->max_brightness = 1;
> + init_data.devicename = dev_name(&phydev->mdio.dev);
> + init_data.fwnode = of_fwnode_handle(led);
> +
> + err = devm_led_classdev_register_ext(dev, cdev, &init_data);
Since init_data.devname_mandatory is false, devicename is ignored.
Which is probably good, becuse the device name of a mdio device is
often ugly, taken from devicetree or switch drivers, for example:
f1072004.mdio-mii
fixed-0
mv88e6xxx-1
So either don't fill devicename or use devname_mandatory (and maybe
fill devicename with something less ugly, but I guess if we don't have
much choice if we want to keep persistent names).
Without devname_mandatory, the name of the LED classdev will be of the
form
color:function[-function-enumerator],
i.e.
green:lan
amber:lan-1
With multiple switch ethenret ports all having LAN function, it is
worth noting that the function enumerator must be explicitly used in the
devicetree, otherwise multiple LEDs will be registered under the same
name, and the LED subsystem will add a number at the and of the name
(function led_classdev_next_name), resulting in names
green:lan
green:lan_1
green:lan_2
...
These names are dependent on the order of classdev registration.
Marek
Hello Christian, also Rob Herring, Andrew Lunn and Pavel Machek,
On Fri, 17 Mar 2023 03:31:21 +0100
Christian Marangi <[email protected]> wrote:
> Add LEDs definition example for qca8k Switch Family to describe how they
> should be defined for a correct usage.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> .../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> index 389892592aac..2e9c14af0223 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> @@ -18,6 +18,8 @@ description:
> PHY it is connected to. In this config, an internal mdio-bus is registered and
> the MDIO master is used for communication. Mixed external and internal
> mdio-bus configurations are not supported by the hardware.
> + Each phy has at least 3 LEDs connected and can be declared
> + using the standard LEDs structure.
>
> properties:
> compatible:
> @@ -117,6 +119,7 @@ unevaluatedProperties: false
> examples:
> - |
> #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/leds/common.h>
>
> mdio {
> #address-cells = <1>;
> @@ -226,6 +229,27 @@ examples:
> label = "lan1";
> phy-mode = "internal";
> phy-handle = <&internal_phy_port1>;
> +
> + leds {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led@0 {
> + reg = <0>;
> + color = <LED_COLOR_ID_WHITE>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <1>;
> + default-state = "keep";
> + };
> +
> + led@1 {
> + reg = <1>;
> + color = <LED_COLOR_ID_AMBER>;
> + function = LED_FUNCTION_LAN;
> + function-enumerator = <1>;
> + default-state = "keep";
> + };
> + };
> };
I have nothing against this, but I would like to point out the
existence of the trigger-sources DT property, and I would like to
discuss how this property should be used by the LED subsystem to choose
default behaviour of a LED.
Consider that we want to specify in device-tree that a PHY LED (or any
other LED) should blink on network activity of the network device
connected to this PHY (let's say the attached network device is eth0).
(Why would we want to specify this in devicetree? Because currently the
drivers either keep the behaviour from boot or change it to something
specific that is not configurable.)
We could specify in DT something like:
eth0: ethernet-controller {
...
}
ethernet-phy {
leds {
led@0 {
reg = <0>;
color = <LED_COLOR_ID_GREEN>;
trigger-sources = <ð0>;
function = LED_FUNCTION_ ?????? ;
}
}
}
The above example specifies that the LED has a trigger source (eth0),
but we still need to specify the trigger itself (for example that
the LED should blink on activity, or the different kinds of link). In my
opinion, this should be specified by the function property, but this
property is currently used in other way: it is filled in with something
like "wan" or "lan" or "wlan", an information which, IMO,
should instead come from the devicename part of the LED, not the
function part.
Recall that the LED names are of the form
devicename:color:function
where the devicename part is supposed to be something like mmc0 or
sda1. With LEDs that are associated with network devices I think the
corresponding name should be the name of the network device (like eth0),
but there is the problem of network namespaces and also that network
devices can be renamed :(.
So one option how to specify the behaviour of the LED to blink on
activity would be to set
function = LED_FUNCTION_ACTIVITY;
but this would conflict with how currently some devicetrees use "lan",
"wlan" or "wan" as the function (which is IMO incorrect, as I said
above).
Another option would be to ignore the function and instead use
additional argument in the trigger-source property, something like
trigger-sources = <ð0 TRIGGER_SOURCE_ACTIVITY>;
I would like to start a discussion on this and hear about your opinions,
because I think that the trigger-sources and function properties were
proposed in good faith, but currently the implementation and usage is a
mess.
Marek
On Fri, Mar 17, 2023 at 03:31:13AM +0100, Christian Marangi wrote:
> Add LEDs basic support for qca8k Switch Family by adding basic
> brightness_set() support.
>
> Since these LEDs refelect port status, the default label is set to
> ":port". DT binding should describe the color, function and number of
> the leds using standard LEDs api.
>
> These LEDs supports only blocking variant of the brightness_set()
> function since they can sleep during access of the switch leds to set
> the brightness.
>
> While at it add to the qca8k header file each mode defined by the Switch
> Documentation for future use.
>
> Signed-off-by: Christian Marangi <[email protected]>
Hi Christian,
Please find my comments inline.
Thanks,
Michal
> ---
> drivers/net/dsa/qca/Kconfig | 8 ++
> drivers/net/dsa/qca/Makefile | 3 +
> drivers/net/dsa/qca/qca8k-8xxx.c | 5 +
> drivers/net/dsa/qca/qca8k-leds.c | 192 +++++++++++++++++++++++++++++++
> drivers/net/dsa/qca/qca8k.h | 59 ++++++++++
> drivers/net/dsa/qca/qca8k_leds.h | 16 +++
> 6 files changed, 283 insertions(+)
> create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
> create mode 100644 drivers/net/dsa/qca/qca8k_leds.h
>
> diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
> index ba339747362c..7a86d6d6a246 100644
> --- a/drivers/net/dsa/qca/Kconfig
> +++ b/drivers/net/dsa/qca/Kconfig
> @@ -15,3 +15,11 @@ config NET_DSA_QCA8K
> help
> This enables support for the Qualcomm Atheros QCA8K Ethernet
> switch chips.
> +
> +config NET_DSA_QCA8K_LEDS_SUPPORT
> + bool "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
> + depends on NET_DSA_QCA8K
> + depends on LEDS_CLASS
> + help
> + This enabled support for LEDs present on the Qualcomm Atheros
> + QCA8K Ethernet switch chips.
> diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
> index 701f1d199e93..ce66b1984e5f 100644
> --- a/drivers/net/dsa/qca/Makefile
> +++ b/drivers/net/dsa/qca/Makefile
> @@ -2,3 +2,6 @@
> obj-$(CONFIG_NET_DSA_AR9331) += ar9331.o
> obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
> qca8k-y += qca8k-common.o qca8k-8xxx.o
> +ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
> +qca8k-y += qca8k-leds.o
> +endif
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 8dfc5db84700..5decf6fe3832 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -22,6 +22,7 @@
> #include <linux/dsa/tag_qca.h>
>
> #include "qca8k.h"
> +#include "qca8k_leds.h"
>
> static void
> qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
> @@ -1727,6 +1728,10 @@ qca8k_setup(struct dsa_switch *ds)
> if (ret)
> return ret;
>
> + ret = qca8k_setup_led_ctrl(priv);
> + if (ret)
> + return ret;
> +
> qca8k_setup_pcs(priv, &priv->pcs_port_0, 0);
> qca8k_setup_pcs(priv, &priv->pcs_port_6, 6);
>
> diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
> new file mode 100644
> index 000000000000..adbe7f6e2994
> --- /dev/null
> +++ b/drivers/net/dsa/qca/qca8k-leds.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/regmap.h>
> +#include <net/dsa.h>
> +
> +#include "qca8k.h"
> +#include "qca8k_leds.h"
> +
> +static int
> +qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
> +{
> + switch (port_num) {
> + case 0:
> + reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> + reg_info->shift = QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT;
> + break;
> + case 1:
> + case 2:
> + case 3:
> + /* Port 123 are controlled on a different reg */
> + reg_info->reg = QCA8K_LED_CTRL_REG(3);
> + reg_info->shift = QCA8K_LED_PHY123_PATTERN_EN_SHIFT(port_num, led_num);
> + break;
> + case 4:
> + reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> + reg_info->shift = QCA8K_LED_PHY4_CONTROL_RULE_SHIFT;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +qca8k_led_brightness_set(struct qca8k_led *led,
> + enum led_brightness brightness)
> +{
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> + u32 mask, val = QCA8K_LED_ALWAYS_OFF;
Nitpick: RCT
> +
> + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> +
> + if (brightness)
> + val = QCA8K_LED_ALWAYS_ON;
> +
> + if (led->port_num == 0 || led->port_num == 4) {
> + mask = QCA8K_LED_PATTERN_EN_MASK;
> + val <<= QCA8K_LED_PATTERN_EN_SHIFT;
> + } else {
> + mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
> + }
> +
> + return regmap_update_bits(priv->regmap, reg_info.reg,
> + mask << reg_info.shift,
> + val << reg_info.shift);
> +}
> +
> +static int
> +qca8k_cled_brightness_set_blocking(struct led_classdev *ldev,
> + enum led_brightness brightness)
> +{
> + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> + return qca8k_led_brightness_set(led, brightness);
> +}
> +
> +static enum led_brightness
> +qca8k_led_brightness_get(struct qca8k_led *led)
> +{
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> + u32 val;
> + int ret;
> +
> + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> +
> + ret = regmap_read(priv->regmap, reg_info.reg, &val);
> + if (ret)
> + return 0;
> +
> + val >>= reg_info.shift;
> +
> + if (led->port_num == 0 || led->port_num == 4) {
> + val &= QCA8K_LED_PATTERN_EN_MASK;
> + val >>= QCA8K_LED_PATTERN_EN_SHIFT;
> + } else {
> + val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
> + }
> +
> + /* Assume brightness ON only when the LED is set to always ON */
> + return val == QCA8K_LED_ALWAYS_ON;
> +}
> +
> +static int
> +qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
> +{
> + struct fwnode_handle *led = NULL, *leds = NULL;
> + struct led_init_data init_data = { };
> + enum led_default_state state;
> + struct qca8k_led *port_led;
> + int led_num, port_index;
> + int ret;
> +
> + leds = fwnode_get_named_child_node(port, "leds");
> + if (!leds) {
> + dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
> + port_num);
> + return 0;
> + }
> +
> + fwnode_for_each_child_node(leds, led) {
> + /* Reg represent the led number of the port.
> + * Each port can have at least 3 leds attached
> + * Commonly:
> + * 1. is gigabit led
> + * 2. is mbit led
> + * 3. additional status led
> + */
> + if (fwnode_property_read_u32(led, "reg", &led_num))
> + continue;
> +
> + if (led_num >= QCA8K_LED_PORT_COUNT) {
> + dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
> + continue;
> + }
In the comment above you say "each port can have AT LEAST 3 leds".
However, now it seems that if the port has more than 3 leds, all the
remaining leds are not initialized.
Is this intentional? If so, maybe it is worth describing in the comment
that for ports with more than 3 leds, only the first 3 leds are
initialized?
According to the code it looks like the port can have up to 3 leds.
> +
> + port_index = 3 * port_num + led_num;
Can QCA8K_LED_PORT_COUNT be used instead of "3"? I guess it is the number
of LEDs per port.
> +
> + port_led = &priv->ports_led[port_index];
Also, the name of the "port_index" variable seems confusing to me. It is
not an index of the port, but rather a unique index of the LED across
all ports, right?
> + port_led->port_num = port_num;
> + port_led->led_num = led_num;
> + port_led->priv = priv;
> +
> + state = led_init_default_state_get(led);
> + switch (state) {
> + case LEDS_DEFSTATE_ON:
> + port_led->cdev.brightness = 1;
> + qca8k_led_brightness_set(port_led, 1);
> + break;
> + case LEDS_DEFSTATE_KEEP:
> + port_led->cdev.brightness =
> + qca8k_led_brightness_get(port_led);
> + break;
> + default:
> + port_led->cdev.brightness = 0;
> + qca8k_led_brightness_set(port_led, 0);
> + }
> +
> + port_led->cdev.max_brightness = 1;
> + port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
> + init_data.default_label = ":port";
> + init_data.devicename = "qca8k";
> + init_data.fwnode = led;
> +
> + ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
> + if (ret)
> + dev_warn(priv->dev, "Failed to int led");
Typo: "init".
How about adding an index of the LED that could not be initialized?
> + }
> +
> + return 0;
> +}
> +
> +int
> +qca8k_setup_led_ctrl(struct qca8k_priv *priv)
> +{
> + struct fwnode_handle *ports, *port;
> + int port_num;
> + int ret;
> +
> + ports = device_get_named_child_node(priv->dev, "ports");
> + if (!ports) {
> + dev_info(priv->dev, "No ports node specified in device tree!\n");
> + return 0;
> + }
> +
> + fwnode_for_each_child_node(ports, port) {
> + if (fwnode_property_read_u32(port, "reg", &port_num))
> + continue;
> +
> + /* Each port can have at least 3 different leds attached.
> + * Switch port starts from 0 to 6, but port 0 and 6 are CPU
> + * port. The port index needs to be decreased by one to identify
> + * the correct port for LED setup.
> + */
Again, are there really "at least 3 different leds" per port?
It's confusing a little bit, because QCA8K_LED_PORT_COUNT == 3, so I
would say it cannot have more than 3.
> + ret = qca8k_parse_port_leds(priv, port, qca8k_port_to_phy(port_num));
As I checked, the function "qca8k_port_to_phy()" can return all 0xFFs
for port_num == 0. Then, this value is implicitly casted to int (as the
last parameter of "qca8k_parse_port_leds()"). Internally, in
"qca8k_parse_port_leds()" this parameter can be used to do some
computing - that looks dangerous.
In summary, I think a special check for CPU port_num == 0 should be
added.
(I guess the LED configuration i only makes sense for non-CPU ports? It
seems you want to configure up to 15 LEDs in total for 5 ports).
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> index 4e48e4dd8b0f..3c3c072fa9c2 100644
> --- a/drivers/net/dsa/qca/qca8k.h
> +++ b/drivers/net/dsa/qca/qca8k.h
> @@ -11,6 +11,7 @@
> #include <linux/delay.h>
> #include <linux/regmap.h>
> #include <linux/gpio.h>
> +#include <linux/leds.h>
> #include <linux/dsa/tag_qca.h>
>
> #define QCA8K_ETHERNET_MDIO_PRIORITY 7
> @@ -85,6 +86,50 @@
> #define QCA8K_MDIO_MASTER_DATA(x) FIELD_PREP(QCA8K_MDIO_MASTER_DATA_MASK, x)
> #define QCA8K_MDIO_MASTER_MAX_PORTS 5
> #define QCA8K_MDIO_MASTER_MAX_REG 32
> +
> +/* LED control register */
> +#define QCA8K_LED_COUNT 15
> +#define QCA8K_LED_PORT_COUNT 3
> +#define QCA8K_LED_RULE_COUNT 6
> +#define QCA8K_LED_RULE_MAX 11
> +
> +#define QCA8K_LED_PHY123_PATTERN_EN_SHIFT(_phy, _led) ((((_phy) - 1) * 6) + 8 + (2 * (_led)))
> +#define QCA8K_LED_PHY123_PATTERN_EN_MASK GENMASK(1, 0)
> +
> +#define QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT 0
> +#define QCA8K_LED_PHY4_CONTROL_RULE_SHIFT 16
> +
> +#define QCA8K_LED_CTRL_REG(_i) (0x050 + (_i) * 4)
> +#define QCA8K_LED_CTRL0_REG 0x50
> +#define QCA8K_LED_CTRL1_REG 0x54
> +#define QCA8K_LED_CTRL2_REG 0x58
> +#define QCA8K_LED_CTRL3_REG 0x5C
> +#define QCA8K_LED_CTRL_SHIFT(_i) (((_i) % 2) * 16)
> +#define QCA8K_LED_CTRL_MASK GENMASK(15, 0)
> +#define QCA8K_LED_RULE_MASK GENMASK(13, 0)
> +#define QCA8K_LED_BLINK_FREQ_MASK GENMASK(1, 0)
> +#define QCA8K_LED_BLINK_FREQ_SHITF 0
> +#define QCA8K_LED_BLINK_2HZ 0
> +#define QCA8K_LED_BLINK_4HZ 1
> +#define QCA8K_LED_BLINK_8HZ 2
> +#define QCA8K_LED_BLINK_AUTO 3
> +#define QCA8K_LED_LINKUP_OVER_MASK BIT(2)
> +#define QCA8K_LED_TX_BLINK_MASK BIT(4)
> +#define QCA8K_LED_RX_BLINK_MASK BIT(5)
> +#define QCA8K_LED_COL_BLINK_MASK BIT(7)
> +#define QCA8K_LED_LINK_10M_EN_MASK BIT(8)
> +#define QCA8K_LED_LINK_100M_EN_MASK BIT(9)
> +#define QCA8K_LED_LINK_1000M_EN_MASK BIT(10)
> +#define QCA8K_LED_POWER_ON_LIGHT_MASK BIT(11)
> +#define QCA8K_LED_HALF_DUPLEX_MASK BIT(12)
> +#define QCA8K_LED_FULL_DUPLEX_MASK BIT(13)
> +#define QCA8K_LED_PATTERN_EN_MASK GENMASK(15, 14)
> +#define QCA8K_LED_PATTERN_EN_SHIFT 14
> +#define QCA8K_LED_ALWAYS_OFF 0
> +#define QCA8K_LED_ALWAYS_BLINK_4HZ 1
> +#define QCA8K_LED_ALWAYS_ON 2
> +#define QCA8K_LED_RULE_CONTROLLED 3
> +
> #define QCA8K_GOL_MAC_ADDR0 0x60
> #define QCA8K_GOL_MAC_ADDR1 0x64
> #define QCA8K_MAX_FRAME_SIZE 0x78
> @@ -383,6 +428,19 @@ struct qca8k_pcs {
> int port;
> };
>
> +struct qca8k_led_pattern_en {
> + u32 reg;
> + u8 shift;
> +};
> +
> +struct qca8k_led {
> + u8 port_num;
> + u8 led_num;
> + u16 old_rule;
> + struct qca8k_priv *priv;
> + struct led_classdev cdev;
> +};
> +
> struct qca8k_priv {
> u8 switch_id;
> u8 switch_revision;
> @@ -407,6 +465,7 @@ struct qca8k_priv {
> struct qca8k_pcs pcs_port_0;
> struct qca8k_pcs pcs_port_6;
> const struct qca8k_match_data *info;
> + struct qca8k_led ports_led[QCA8K_LED_COUNT];
> };
>
> struct qca8k_mib_desc {
> diff --git a/drivers/net/dsa/qca/qca8k_leds.h b/drivers/net/dsa/qca/qca8k_leds.h
> new file mode 100644
> index 000000000000..ab367f05b173
> --- /dev/null
> +++ b/drivers/net/dsa/qca/qca8k_leds.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __QCA8K_LEDS_H
> +#define __QCA8K_LEDS_H
> +
> +/* Leds Support function */
> +#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
> +int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
> +#else
> +static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
> +{
> + return 0;
> +}
> +#endif
> +
> +#endif /* __QCA8K_LEDS_H */
> --
> 2.39.2
>
On Fri, Mar 17, 2023 at 03:31:14AM +0100, Christian Marangi wrote:
> Add LEDs blink_set() support to qca8k Switch Family.
> These LEDs support hw accellerated blinking at a fixed rate
> of 4Hz.
>
> Reject any other value since not supported by the LEDs switch.
>
> Signed-off-by: Christian Marangi <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
> ---
> drivers/net/dsa/qca/qca8k-leds.c | 38 ++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
> index adbe7f6e2994..c229575c7e8c 100644
> --- a/drivers/net/dsa/qca/qca8k-leds.c
> +++ b/drivers/net/dsa/qca/qca8k-leds.c
> @@ -92,6 +92,43 @@ qca8k_led_brightness_get(struct qca8k_led *led)
> return val == QCA8K_LED_ALWAYS_ON;
> }
>
> +static int
> +qca8k_cled_blink_set(struct led_classdev *ldev,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> +{
> + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> + u32 mask, val = QCA8K_LED_ALWAYS_BLINK_4HZ;
> + struct qca8k_led_pattern_en reg_info;
> + struct qca8k_priv *priv = led->priv;
> +
> + if (*delay_on == 0 && *delay_off == 0) {
> + *delay_on = 125;
> + *delay_off = 125;
> + }
> +
> + if (*delay_on != 125 || *delay_off != 125) {
> + /* The hardware only supports blinking at 4Hz. Fall back
> + * to software implementation in other cases.
> + */
> + return -EINVAL;
> + }
> +
> + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> +
> + if (led->port_num == 0 || led->port_num == 4) {
> + mask = QCA8K_LED_PATTERN_EN_MASK;
> + val <<= QCA8K_LED_PATTERN_EN_SHIFT;
> + } else {
> + mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
> + }
Could you add a comment as to why the HW requires different approaches for
inner and outer ports?
> +
> + regmap_update_bits(priv->regmap, reg_info.reg, mask << reg_info.shift,
> + val << reg_info.shift);
> +
> + return 0;
> +}
> +
> static int
> qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
> {
> @@ -149,6 +186,7 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p
>
> port_led->cdev.max_brightness = 1;
> port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
> + port_led->cdev.blink_set = qca8k_cled_blink_set;
> init_data.default_label = ":port";
> init_data.devicename = "qca8k";
> init_data.fwnode = led;
> --
> 2.39.2
>
Thanks,
Michal
> (I guess the LED configuration i only makes sense for non-CPU ports? It
> seems you want to configure up to 15 LEDs in total for 5 ports).
Hi Michal
In the general case, there is no reason that i can think that stops
the CPU port having LEDs. For many switch designs, there is no
specific CPU port, any can be used. And all ports are likely to have
an LED controller.
What becomes tricky with Linux is offloading blinking to CPU ports.
There is no netdev to represent it, hence no netdev based software
blinking. And without software blinking, you have nothing to offload
to hardware. But you could still use the LEDs for other things.
Having said all that, i don't think i have ever seen a box with LEDs
for the CPU port.
Andrew
On Fri, Mar 17, 2023 at 03:31:15AM +0100, Christian Marangi wrote:
> From: Andrew Lunn <[email protected]>
>
> Define common binding parsing for all PHY drivers with LEDs using
> phylib. Parse the DT as part of the phy_probe and add LEDs to the
> linux LED class infrastructure. For the moment, provide a dummy
> brightness function, which will later be replaced with a call into the
> PHY driver.
>
Hi Andrew,
Personally, I see no good reason to provide a dummy implementation
of "phy_led_set_brightness", especially if you implement it in the next
patch. You only use that function only the function pointer in
"led_classdev". I think you can just skip it in this patch.
Please find the rest of my comments inline.
Thanks,
Michal
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> drivers/net/phy/Kconfig | 1 +
> drivers/net/phy/phy_device.c | 75 ++++++++++++++++++++++++++++++++++++
> include/linux/phy.h | 16 ++++++++
> 3 files changed, 92 insertions(+)
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index f5df2edc94a5..666efa6b1c8e 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -16,6 +16,7 @@ config PHYLINK
> menuconfig PHYLIB
> tristate "PHY Device support and infrastructure"
> depends on NETDEVICES
> + depends on LEDS_CLASS
> select MDIO_DEVICE
> select MDIO_DEVRES
> help
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 9ba8f973f26f..ee800f93c8c3 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -19,10 +19,12 @@
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> +#include <linux/list.h>
> #include <linux/mdio.h>
> #include <linux/mii.h>
> #include <linux/mm.h>
> #include <linux/module.h>
> +#include <linux/of.h>
> #include <linux/netdevice.h>
> #include <linux/phy.h>
> #include <linux/phy_led_triggers.h>
> @@ -658,6 +660,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
> device_initialize(&mdiodev->dev);
>
> dev->state = PHY_DOWN;
> + INIT_LIST_HEAD(&dev->leds);
>
> mutex_init(&dev->lock);
> INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
> @@ -2964,6 +2967,73 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
> return phydrv->config_intr && phydrv->handle_interrupt;
> }
>
> +/* Dummy implementation until calls into PHY driver are added */
> +static int phy_led_set_brightness(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return 0;
> +}
It can be removed from this patch.
> +
> +static int of_phy_led(struct phy_device *phydev,
> + struct device_node *led)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + struct led_init_data init_data = {};
> + struct led_classdev *cdev;
> + struct phy_led *phyled;
> + int err;
> +
> + phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
> + if (!phyled)
> + return -ENOMEM;
> +
> + cdev = &phyled->led_cdev;
> +
> + err = of_property_read_u32(led, "reg", &phyled->index);
> + if (err)
> + return err;
Memory leak. 'phyled' is not freed in case of error.
> +
> + cdev->brightness_set_blocking = phy_led_set_brightness;
Please move this initialization to the patch where you are actually
implementing this callback.
> + cdev->max_brightness = 1;
> + init_data.devicename = dev_name(&phydev->mdio.dev);
> + init_data.fwnode = of_fwnode_handle(led);
> +
> + err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> + if (err)
> + return err;
Another memory leak.
> +
> + list_add(&phyled->list, &phydev->leds);
Where do you free the memory allocated for phy_led structure?
> +
> + return 0;
> +}
> +
> +static int of_phy_leds(struct phy_device *phydev)
> +{
> + struct device_node *node = phydev->mdio.dev.of_node;
> + struct device_node *leds, *led;
> + int err;
> +
> + if (!IS_ENABLED(CONFIG_OF_MDIO))
> + return 0;
> +
> + if (!node)
> + return 0;
> +
> + leds = of_get_child_by_name(node, "leds");
> + if (!leds)
> + return 0;
> +
> + for_each_available_child_of_node(leds, led) {
> + err = of_phy_led(phydev, led);
> + if (err) {
> + of_node_put(led);
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +
> /**
> * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
> * @fwnode: pointer to the mdio_device's fwnode
> @@ -3142,6 +3212,11 @@ static int phy_probe(struct device *dev)
> /* Set the state to READY by default */
> phydev->state = PHY_READY;
>
> + /* Get the LEDs from the device tree, and instantiate standard
> + * LEDs for them.
> + */
> + err = of_phy_leds(phydev);
> +
> out:
> /* Assert the reset signal */
> if (err)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index fbeba4fee8d4..88a77ff60be9 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -14,6 +14,7 @@
> #include <linux/compiler.h>
> #include <linux/spinlock.h>
> #include <linux/ethtool.h>
> +#include <linux/leds.h>
> #include <linux/linkmode.h>
> #include <linux/netlink.h>
> #include <linux/mdio.h>
> @@ -595,6 +596,7 @@ struct macsec_ops;
> * @phy_num_led_triggers: Number of triggers in @phy_led_triggers
> * @led_link_trigger: LED trigger for link up/down
> * @last_triggered: last LED trigger for link speed
> + * @leds: list of PHY LED structures
> * @master_slave_set: User requested master/slave configuration
> * @master_slave_get: Current master/slave advertisement
> * @master_slave_state: Current master/slave configuration
> @@ -690,6 +692,7 @@ struct phy_device {
>
> struct phy_led_trigger *led_link_trigger;
> #endif
> + struct list_head leds;
>
> /*
> * Interrupt number for this PHY
> @@ -825,6 +828,19 @@ struct phy_plca_status {
> bool pst;
> };
>
> +/**
> + * struct phy_led: An LED driven by the PHY
> + *
> + * @list: List of LEDs
> + * @led_cdev: Standard LED class structure
> + * @index: Number of the LED
> + */
> +struct phy_led {
> + struct list_head list;
> + struct led_classdev led_cdev;
> + u32 index;
> +};
> +
> /**
> * struct phy_driver - Driver structure for a particular PHY type
> *
> --
> 2.39.2
>
On Fri, Mar 17, 2023 at 08:45:19AM +0100, Marek Beh?n wrote:
> On Fri, 17 Mar 2023 03:31:15 +0100
> Christian Marangi <[email protected]> wrote:
>
> > + cdev->brightness_set_blocking = phy_led_set_brightness;
> > + cdev->max_brightness = 1;
> > + init_data.devicename = dev_name(&phydev->mdio.dev);
> > + init_data.fwnode = of_fwnode_handle(led);
> > +
> > + err = devm_led_classdev_register_ext(dev, cdev, &init_data);
>
> Since init_data.devname_mandatory is false, devicename is ignored.
> Which is probably good, becuse the device name of a mdio device is
> often ugly, taken from devicetree or switch drivers, for example:
> f1072004.mdio-mii
> fixed-0
> mv88e6xxx-1
> So either don't fill devicename or use devname_mandatory (and maybe
> fill devicename with something less ugly, but I guess if we don't have
> much choice if we want to keep persistent names).
>
> Without devname_mandatory, the name of the LED classdev will be of the
> form
> color:function[-function-enumerator],
> i.e.
> green:lan
> amber:lan-1
>
> With multiple switch ethenret ports all having LAN function, it is
> worth noting that the function enumerator must be explicitly used in the
> devicetree, otherwise multiple LEDs will be registered under the same
> name, and the LED subsystem will add a number at the and of the name
> (function led_classdev_next_name), resulting in names
> green:lan
> green:lan_1
> green:lan_2
> ...
I'm testing on a Marvell RDK, with limited LEDs. It has one LED on the
front port to represent the WAN port. The DT patch is at the end of
the series. With that, i end up with:
root@370rd:/sys/class/leds# ls -l
total 0
lrwxrwxrwx 1 root root 0 Mar 17 01:10 f1072004.mdio-mii:00:WAN -> ../../devices/platform/soc/soc:interna
l-regs/f1072004.mdio/mdio_bus/f1072004.mdio-mii/f1072004.mdio-mii:00/leds/f1072004.mdio-mii:00:WAN
I also have:
root@370rd:/sys/class/net/eth0/phydev/leds# ls
f1072004.mdio-mii:00:WAN
f1072004.mdio-mii:00: is not nice, but it is unique to a netdev. The
last part then comes from the label property. Since there is only one
LED, i went with what the port is intended to be used as. If there had
been more LEDs, i would of probably used labels like "LINK" and
"ACTIVITY", since that is often what they reset default
to. Alternatively, you could names the "Left" and "Right", which does
suggest they can be given any function.
I don't actually think the name is too important, so long as it is
unique. You are going to find it via /sys/class/net. MAC LEDs should
be /sys/class/net/eth42/leds, and PHY LEDs will be
/sys/class/net/phydev/leds.
It has been discussed in the past to either extend ethtool to
understand this, or write a new little tool to make it easier to
manipulate these LEDs.
Andrew
On Fri, Mar 17, 2023 at 03:31:16AM +0100, Christian Marangi wrote:
> From: Andrew Lunn <[email protected]>
>
> Linux LEDs can be software controlled via the brightness file in /sys.
> LED drivers need to implement a brightness_set function which the core
> will call. Implement an intermediary in phy_device, which will call
> into the phy driver if it implements the necessary function.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
As I have already mentioned in my comments for the patch 4, the final
version of "phy_led_set_brightness()" can appear here (without
an intermediate step with a dummy implementation).
Thanks,
Michal
> drivers/net/phy/phy_device.c | 15 ++++++++++++---
> include/linux/phy.h | 11 +++++++++++
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index ee800f93c8c3..c7312a9e820d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2967,11 +2967,18 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
> return phydrv->config_intr && phydrv->handle_interrupt;
> }
>
> -/* Dummy implementation until calls into PHY driver are added */
> static int phy_led_set_brightness(struct led_classdev *led_cdev,
> enum led_brightness value)
> {
> - return 0;
> + struct phy_led *phyled = to_phy_led(led_cdev);
> + struct phy_device *phydev = phyled->phydev;
> + int err;
> +
> + mutex_lock(&phydev->lock);
> + err = phydev->drv->led_brightness_set(phydev, phyled->index, value);
> + mutex_unlock(&phydev->lock);
> +
> + return err;
> }
>
> static int of_phy_led(struct phy_device *phydev,
> @@ -2988,12 +2995,14 @@ static int of_phy_led(struct phy_device *phydev,
> return -ENOMEM;
>
> cdev = &phyled->led_cdev;
> + phyled->phydev = phydev;
>
> err = of_property_read_u32(led, "reg", &phyled->index);
> if (err)
> return err;
>
> - cdev->brightness_set_blocking = phy_led_set_brightness;
> + if (phydev->drv->led_brightness_set)
> + cdev->brightness_set_blocking = phy_led_set_brightness;
> cdev->max_brightness = 1;
> init_data.devicename = dev_name(&phydev->mdio.dev);
> init_data.fwnode = of_fwnode_handle(led);
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 88a77ff60be9..94fd21d5e145 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -832,15 +832,19 @@ struct phy_plca_status {
> * struct phy_led: An LED driven by the PHY
> *
> * @list: List of LEDs
> + * @phydev: PHY this LED is attached to
> * @led_cdev: Standard LED class structure
> * @index: Number of the LED
> */
> struct phy_led {
> struct list_head list;
> + struct phy_device *phydev;
> struct led_classdev led_cdev;
> u32 index;
> };
>
> +#define to_phy_led(d) container_of(d, struct phy_led, led_cdev)
> +
> /**
> * struct phy_driver - Driver structure for a particular PHY type
> *
> @@ -1063,6 +1067,13 @@ struct phy_driver {
> /** @get_plca_status: Return the current PLCA status info */
> int (*get_plca_status)(struct phy_device *dev,
> struct phy_plca_status *plca_st);
> +
> + /* Set a PHY LED brightness. Index indicates which of the PHYs
> + * led should be set. Value follows the standard LED class meaning,
> + * e.g. LED_OFF, LED_HALF, LED_FULL.
> + */
> + int (*led_brightness_set)(struct phy_device *dev,
> + u32 index, enum led_brightness value);
> };
> #define to_phy_driver(d) container_of(to_mdio_common_driver(d), \
> struct phy_driver, mdiodrv)
> --
> 2.39.2
>
On Fri, Mar 17, 2023 at 12:24:23PM +0100, Michal Kubiak wrote:
> On Fri, Mar 17, 2023 at 03:31:13AM +0100, Christian Marangi wrote:
> > Add LEDs basic support for qca8k Switch Family by adding basic
> > brightness_set() support.
> >
> > Since these LEDs refelect port status, the default label is set to
> > ":port". DT binding should describe the color, function and number of
> > the leds using standard LEDs api.
> >
> > These LEDs supports only blocking variant of the brightness_set()
> > function since they can sleep during access of the switch leds to set
> > the brightness.
> >
> > While at it add to the qca8k header file each mode defined by the Switch
> > Documentation for future use.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
>
> Hi Christian,
>
> Please find my comments inline.
>
> Thanks,
> Michal
>
> > ---
> > drivers/net/dsa/qca/Kconfig | 8 ++
> > drivers/net/dsa/qca/Makefile | 3 +
> > drivers/net/dsa/qca/qca8k-8xxx.c | 5 +
> > drivers/net/dsa/qca/qca8k-leds.c | 192 +++++++++++++++++++++++++++++++
> > drivers/net/dsa/qca/qca8k.h | 59 ++++++++++
> > drivers/net/dsa/qca/qca8k_leds.h | 16 +++
> > 6 files changed, 283 insertions(+)
> > create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
> > create mode 100644 drivers/net/dsa/qca/qca8k_leds.h
> >
> > diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
> > index ba339747362c..7a86d6d6a246 100644
> > --- a/drivers/net/dsa/qca/Kconfig
> > +++ b/drivers/net/dsa/qca/Kconfig
> > @@ -15,3 +15,11 @@ config NET_DSA_QCA8K
> > help
> > This enables support for the Qualcomm Atheros QCA8K Ethernet
> > switch chips.
> > +
> > +config NET_DSA_QCA8K_LEDS_SUPPORT
> > + bool "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
> > + depends on NET_DSA_QCA8K
> > + depends on LEDS_CLASS
> > + help
> > + This enabled support for LEDs present on the Qualcomm Atheros
> > + QCA8K Ethernet switch chips.
> > diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
> > index 701f1d199e93..ce66b1984e5f 100644
> > --- a/drivers/net/dsa/qca/Makefile
> > +++ b/drivers/net/dsa/qca/Makefile
> > @@ -2,3 +2,6 @@
> > obj-$(CONFIG_NET_DSA_AR9331) += ar9331.o
> > obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
> > qca8k-y += qca8k-common.o qca8k-8xxx.o
> > +ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
> > +qca8k-y += qca8k-leds.o
> > +endif
> > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> > index 8dfc5db84700..5decf6fe3832 100644
> > --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> > @@ -22,6 +22,7 @@
> > #include <linux/dsa/tag_qca.h>
> >
> > #include "qca8k.h"
> > +#include "qca8k_leds.h"
> >
> > static void
> > qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
> > @@ -1727,6 +1728,10 @@ qca8k_setup(struct dsa_switch *ds)
> > if (ret)
> > return ret;
> >
> > + ret = qca8k_setup_led_ctrl(priv);
> > + if (ret)
> > + return ret;
> > +
> > qca8k_setup_pcs(priv, &priv->pcs_port_0, 0);
> > qca8k_setup_pcs(priv, &priv->pcs_port_6, 6);
> >
> > diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
> > new file mode 100644
> > index 000000000000..adbe7f6e2994
> > --- /dev/null
> > +++ b/drivers/net/dsa/qca/qca8k-leds.c
> > @@ -0,0 +1,192 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/regmap.h>
> > +#include <net/dsa.h>
> > +
> > +#include "qca8k.h"
> > +#include "qca8k_leds.h"
> > +
> > +static int
> > +qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
> > +{
> > + switch (port_num) {
> > + case 0:
> > + reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> > + reg_info->shift = QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT;
> > + break;
> > + case 1:
> > + case 2:
> > + case 3:
> > + /* Port 123 are controlled on a different reg */
> > + reg_info->reg = QCA8K_LED_CTRL_REG(3);
> > + reg_info->shift = QCA8K_LED_PHY123_PATTERN_EN_SHIFT(port_num, led_num);
> > + break;
> > + case 4:
> > + reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> > + reg_info->shift = QCA8K_LED_PHY4_CONTROL_RULE_SHIFT;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +qca8k_led_brightness_set(struct qca8k_led *led,
> > + enum led_brightness brightness)
> > +{
> > + struct qca8k_led_pattern_en reg_info;
> > + struct qca8k_priv *priv = led->priv;
> > + u32 mask, val = QCA8K_LED_ALWAYS_OFF;
>
> Nitpick: RCT
>
> > +
> > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > +
> > + if (brightness)
> > + val = QCA8K_LED_ALWAYS_ON;
> > +
> > + if (led->port_num == 0 || led->port_num == 4) {
> > + mask = QCA8K_LED_PATTERN_EN_MASK;
> > + val <<= QCA8K_LED_PATTERN_EN_SHIFT;
> > + } else {
> > + mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
> > + }
> > +
> > + return regmap_update_bits(priv->regmap, reg_info.reg,
> > + mask << reg_info.shift,
> > + val << reg_info.shift);
> > +}
> > +
> > +static int
> > +qca8k_cled_brightness_set_blocking(struct led_classdev *ldev,
> > + enum led_brightness brightness)
> > +{
> > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > + return qca8k_led_brightness_set(led, brightness);
> > +}
> > +
> > +static enum led_brightness
> > +qca8k_led_brightness_get(struct qca8k_led *led)
> > +{
> > + struct qca8k_led_pattern_en reg_info;
> > + struct qca8k_priv *priv = led->priv;
> > + u32 val;
> > + int ret;
> > +
> > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > +
> > + ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > + if (ret)
> > + return 0;
> > +
> > + val >>= reg_info.shift;
> > +
> > + if (led->port_num == 0 || led->port_num == 4) {
> > + val &= QCA8K_LED_PATTERN_EN_MASK;
> > + val >>= QCA8K_LED_PATTERN_EN_SHIFT;
> > + } else {
> > + val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
> > + }
> > +
> > + /* Assume brightness ON only when the LED is set to always ON */
> > + return val == QCA8K_LED_ALWAYS_ON;
> > +}
> > +
> > +static int
> > +qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
> > +{
> > + struct fwnode_handle *led = NULL, *leds = NULL;
> > + struct led_init_data init_data = { };
> > + enum led_default_state state;
> > + struct qca8k_led *port_led;
> > + int led_num, port_index;
> > + int ret;
> > +
> > + leds = fwnode_get_named_child_node(port, "leds");
> > + if (!leds) {
> > + dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
> > + port_num);
> > + return 0;
> > + }
> > +
> > + fwnode_for_each_child_node(leds, led) {
> > + /* Reg represent the led number of the port.
> > + * Each port can have at least 3 leds attached
> > + * Commonly:
> > + * 1. is gigabit led
> > + * 2. is mbit led
> > + * 3. additional status led
> > + */
> > + if (fwnode_property_read_u32(led, "reg", &led_num))
> > + continue;
> > +
> > + if (led_num >= QCA8K_LED_PORT_COUNT) {
> > + dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
> > + continue;
> > + }
>
> In the comment above you say "each port can have AT LEAST 3 leds".
> However, now it seems that if the port has more than 3 leds, all the
> remaining leds are not initialized.
> Is this intentional? If so, maybe it is worth describing in the comment
> that for ports with more than 3 leds, only the first 3 leds are
> initialized?
>
> According to the code it looks like the port can have up to 3 leds.
>
Think I should rework the comments and make them more direct/simple.
qca8k switch have a max of 5 port.
each port CAN have a max of 3 leds connected.
It's really a limitation of pin on the switch chip and hw regs so the
situation can't happen.
> > +
> > + port_index = 3 * port_num + led_num;
>
> Can QCA8K_LED_PORT_COUNT be used instead of "3"? I guess it is the number
> of LEDs per port.
>
This variable it's really to make it easier to reference the led in the
priv struct. If asked I can rework this to an array of array (one per
port and each port out of 3 possigle LED).
> > +
> > + port_led = &priv->ports_led[port_index];
>
> Also, the name of the "port_index" variable seems confusing to me. It is
> not an index of the port, but rather a unique index of the LED across
> all ports, right?
>
As said above, they are unique index that comes from port and LED of the
port. Really something to represent the code easier internally.
> > + port_led->port_num = port_num;
> > + port_led->led_num = led_num;
> > + port_led->priv = priv;
> > +
> > + state = led_init_default_state_get(led);
> > + switch (state) {
> > + case LEDS_DEFSTATE_ON:
> > + port_led->cdev.brightness = 1;
> > + qca8k_led_brightness_set(port_led, 1);
> > + break;
> > + case LEDS_DEFSTATE_KEEP:
> > + port_led->cdev.brightness =
> > + qca8k_led_brightness_get(port_led);
> > + break;
> > + default:
> > + port_led->cdev.brightness = 0;
> > + qca8k_led_brightness_set(port_led, 0);
> > + }
> > +
> > + port_led->cdev.max_brightness = 1;
> > + port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
> > + init_data.default_label = ":port";
> > + init_data.devicename = "qca8k";
> > + init_data.fwnode = led;
> > +
> > + ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
> > + if (ret)
> > + dev_warn(priv->dev, "Failed to int led");
>
> Typo: "init".
> How about adding an index of the LED that could not be initialized?
>
Ok will add more info in the port and led that failed to init.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int
> > +qca8k_setup_led_ctrl(struct qca8k_priv *priv)
> > +{
> > + struct fwnode_handle *ports, *port;
> > + int port_num;
> > + int ret;
> > +
> > + ports = device_get_named_child_node(priv->dev, "ports");
> > + if (!ports) {
> > + dev_info(priv->dev, "No ports node specified in device tree!\n");
> > + return 0;
> > + }
> > +
> > + fwnode_for_each_child_node(ports, port) {
> > + if (fwnode_property_read_u32(port, "reg", &port_num))
> > + continue;
> > +
> > + /* Each port can have at least 3 different leds attached.
> > + * Switch port starts from 0 to 6, but port 0 and 6 are CPU
> > + * port. The port index needs to be decreased by one to identify
> > + * the correct port for LED setup.
> > + */
>
> Again, are there really "at least 3 different leds" per port?
> It's confusing a little bit, because QCA8K_LED_PORT_COUNT == 3, so I
> would say it cannot have more than 3.
>
> > + ret = qca8k_parse_port_leds(priv, port, qca8k_port_to_phy(port_num));
>
> As I checked, the function "qca8k_port_to_phy()" can return all 0xFFs
> for port_num == 0. Then, this value is implicitly casted to int (as the
> last parameter of "qca8k_parse_port_leds()"). Internally, in
> "qca8k_parse_port_leds()" this parameter can be used to do some
> computing - that looks dangerous.
> In summary, I think a special check for CPU port_num == 0 should be
> added.
> (I guess the LED configuration i only makes sense for non-CPU ports? It
> seems you want to configure up to 15 LEDs in total for 5 ports).
>
IMHO for this, we can ignore handling this corner case. The hw doesn't
supports leds for port0 and port6 (the 2 CPU port) so the case won't
ever apply. But if asked I can add the case, not that it will cause any
problem in how the regs and shift are referenced in the code.
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> > index 4e48e4dd8b0f..3c3c072fa9c2 100644
> > --- a/drivers/net/dsa/qca/qca8k.h
> > +++ b/drivers/net/dsa/qca/qca8k.h
> > @@ -11,6 +11,7 @@
> > #include <linux/delay.h>
> > #include <linux/regmap.h>
> > #include <linux/gpio.h>
> > +#include <linux/leds.h>
> > #include <linux/dsa/tag_qca.h>
> >
> > #define QCA8K_ETHERNET_MDIO_PRIORITY 7
> > @@ -85,6 +86,50 @@
> > #define QCA8K_MDIO_MASTER_DATA(x) FIELD_PREP(QCA8K_MDIO_MASTER_DATA_MASK, x)
> > #define QCA8K_MDIO_MASTER_MAX_PORTS 5
> > #define QCA8K_MDIO_MASTER_MAX_REG 32
> > +
> > +/* LED control register */
> > +#define QCA8K_LED_COUNT 15
> > +#define QCA8K_LED_PORT_COUNT 3
> > +#define QCA8K_LED_RULE_COUNT 6
> > +#define QCA8K_LED_RULE_MAX 11
> > +
> > +#define QCA8K_LED_PHY123_PATTERN_EN_SHIFT(_phy, _led) ((((_phy) - 1) * 6) + 8 + (2 * (_led)))
> > +#define QCA8K_LED_PHY123_PATTERN_EN_MASK GENMASK(1, 0)
> > +
> > +#define QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT 0
> > +#define QCA8K_LED_PHY4_CONTROL_RULE_SHIFT 16
> > +
> > +#define QCA8K_LED_CTRL_REG(_i) (0x050 + (_i) * 4)
> > +#define QCA8K_LED_CTRL0_REG 0x50
> > +#define QCA8K_LED_CTRL1_REG 0x54
> > +#define QCA8K_LED_CTRL2_REG 0x58
> > +#define QCA8K_LED_CTRL3_REG 0x5C
> > +#define QCA8K_LED_CTRL_SHIFT(_i) (((_i) % 2) * 16)
> > +#define QCA8K_LED_CTRL_MASK GENMASK(15, 0)
> > +#define QCA8K_LED_RULE_MASK GENMASK(13, 0)
> > +#define QCA8K_LED_BLINK_FREQ_MASK GENMASK(1, 0)
> > +#define QCA8K_LED_BLINK_FREQ_SHITF 0
> > +#define QCA8K_LED_BLINK_2HZ 0
> > +#define QCA8K_LED_BLINK_4HZ 1
> > +#define QCA8K_LED_BLINK_8HZ 2
> > +#define QCA8K_LED_BLINK_AUTO 3
> > +#define QCA8K_LED_LINKUP_OVER_MASK BIT(2)
> > +#define QCA8K_LED_TX_BLINK_MASK BIT(4)
> > +#define QCA8K_LED_RX_BLINK_MASK BIT(5)
> > +#define QCA8K_LED_COL_BLINK_MASK BIT(7)
> > +#define QCA8K_LED_LINK_10M_EN_MASK BIT(8)
> > +#define QCA8K_LED_LINK_100M_EN_MASK BIT(9)
> > +#define QCA8K_LED_LINK_1000M_EN_MASK BIT(10)
> > +#define QCA8K_LED_POWER_ON_LIGHT_MASK BIT(11)
> > +#define QCA8K_LED_HALF_DUPLEX_MASK BIT(12)
> > +#define QCA8K_LED_FULL_DUPLEX_MASK BIT(13)
> > +#define QCA8K_LED_PATTERN_EN_MASK GENMASK(15, 14)
> > +#define QCA8K_LED_PATTERN_EN_SHIFT 14
> > +#define QCA8K_LED_ALWAYS_OFF 0
> > +#define QCA8K_LED_ALWAYS_BLINK_4HZ 1
> > +#define QCA8K_LED_ALWAYS_ON 2
> > +#define QCA8K_LED_RULE_CONTROLLED 3
> > +
> > #define QCA8K_GOL_MAC_ADDR0 0x60
> > #define QCA8K_GOL_MAC_ADDR1 0x64
> > #define QCA8K_MAX_FRAME_SIZE 0x78
> > @@ -383,6 +428,19 @@ struct qca8k_pcs {
> > int port;
> > };
> >
> > +struct qca8k_led_pattern_en {
> > + u32 reg;
> > + u8 shift;
> > +};
> > +
> > +struct qca8k_led {
> > + u8 port_num;
> > + u8 led_num;
> > + u16 old_rule;
> > + struct qca8k_priv *priv;
> > + struct led_classdev cdev;
> > +};
> > +
> > struct qca8k_priv {
> > u8 switch_id;
> > u8 switch_revision;
> > @@ -407,6 +465,7 @@ struct qca8k_priv {
> > struct qca8k_pcs pcs_port_0;
> > struct qca8k_pcs pcs_port_6;
> > const struct qca8k_match_data *info;
> > + struct qca8k_led ports_led[QCA8K_LED_COUNT];
> > };
> >
> > struct qca8k_mib_desc {
> > diff --git a/drivers/net/dsa/qca/qca8k_leds.h b/drivers/net/dsa/qca/qca8k_leds.h
> > new file mode 100644
> > index 000000000000..ab367f05b173
> > --- /dev/null
> > +++ b/drivers/net/dsa/qca/qca8k_leds.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef __QCA8K_LEDS_H
> > +#define __QCA8K_LEDS_H
> > +
> > +/* Leds Support function */
> > +#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
> > +int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
> > +#else
> > +static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
> > +#endif /* __QCA8K_LEDS_H */
> > --
> > 2.39.2
> >
--
Ansuel
On Fri, Mar 17, 2023 at 12:54:09PM +0100, Michal Kubiak wrote:
> On Fri, Mar 17, 2023 at 03:31:14AM +0100, Christian Marangi wrote:
> > Add LEDs blink_set() support to qca8k Switch Family.
> > These LEDs support hw accellerated blinking at a fixed rate
> > of 4Hz.
> >
> > Reject any other value since not supported by the LEDs switch.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
> > Reviewed-by: Andrew Lunn <[email protected]>
> > ---
> > drivers/net/dsa/qca/qca8k-leds.c | 38 ++++++++++++++++++++++++++++++++
> > 1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
> > index adbe7f6e2994..c229575c7e8c 100644
> > --- a/drivers/net/dsa/qca/qca8k-leds.c
> > +++ b/drivers/net/dsa/qca/qca8k-leds.c
> > @@ -92,6 +92,43 @@ qca8k_led_brightness_get(struct qca8k_led *led)
> > return val == QCA8K_LED_ALWAYS_ON;
> > }
> >
> > +static int
> > +qca8k_cled_blink_set(struct led_classdev *ldev,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off)
> > +{
> > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > + u32 mask, val = QCA8K_LED_ALWAYS_BLINK_4HZ;
> > + struct qca8k_led_pattern_en reg_info;
> > + struct qca8k_priv *priv = led->priv;
> > +
> > + if (*delay_on == 0 && *delay_off == 0) {
> > + *delay_on = 125;
> > + *delay_off = 125;
> > + }
> > +
> > + if (*delay_on != 125 || *delay_off != 125) {
> > + /* The hardware only supports blinking at 4Hz. Fall back
> > + * to software implementation in other cases.
> > + */
> > + return -EINVAL;
> > + }
> > +
> > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > +
> > + if (led->port_num == 0 || led->port_num == 4) {
> > + mask = QCA8K_LED_PATTERN_EN_MASK;
> > + val <<= QCA8K_LED_PATTERN_EN_SHIFT;
> > + } else {
> > + mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
> > + }
>
> Could you add a comment as to why the HW requires different approaches for
> inner and outer ports?
>
Will add some comments, but for reference, it's really just how the
switch regs setup these and provide access and configuration from them.
phy0 and phy4 are in a reg,
phy1-2-3 are in a separate table with different shift and mask.
> > +
> > + regmap_update_bits(priv->regmap, reg_info.reg, mask << reg_info.shift,
> > + val << reg_info.shift);
> > +
> > + return 0;
> > +}
> > +
> > static int
> > qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
> > {
> > @@ -149,6 +186,7 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p
> >
> > port_led->cdev.max_brightness = 1;
> > port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
> > + port_led->cdev.blink_set = qca8k_cled_blink_set;
> > init_data.default_label = ":port";
> > init_data.devicename = "qca8k";
> > init_data.fwnode = led;
> > --
> > 2.39.2
> >
>
>
> Thanks,
> Michal
--
Ansuel
On Fri, Mar 17, 2023 at 02:38:15PM +0100, Michal Kubiak wrote:
> On Fri, Mar 17, 2023 at 03:31:15AM +0100, Christian Marangi wrote:
> > From: Andrew Lunn <[email protected]>
> >
> > Define common binding parsing for all PHY drivers with LEDs using
> > phylib. Parse the DT as part of the phy_probe and add LEDs to the
> > linux LED class infrastructure. For the moment, provide a dummy
> > brightness function, which will later be replaced with a call into the
> > PHY driver.
> >
>
> Hi Andrew,
>
> Personally, I see no good reason to provide a dummy implementation
> of "phy_led_set_brightness", especially if you implement it in the next
> patch. You only use that function only the function pointer in
> "led_classdev". I think you can just skip it in this patch.
Hi Michal
The basic code for this patch has been sitting in my tree for a long
time. It used to be, if you did not have a set_brightness method in
cdev, the registration failed. That made it hard to test this patch on
its own during development work, did i have the link list correct, can
i unload the PHY driver without it exploding etc. I need to check if
it is still mandatory.
> > +static int of_phy_led(struct phy_device *phydev,
> > + struct device_node *led)
> > +{
> > + struct device *dev = &phydev->mdio.dev;
> > + struct led_init_data init_data = {};
> > + struct led_classdev *cdev;
> > + struct phy_led *phyled;
> > + int err;
> > +
> > + phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
> > + if (!phyled)
> > + return -ENOMEM;
> > +
> > + cdev = &phyled->led_cdev;
> > +
> > + err = of_property_read_u32(led, "reg", &phyled->index);
> > + if (err)
> > + return err;
>
> Memory leak. 'phyled' is not freed in case of error.
devm_ API, so it gets freed when the probe fails.
> > +
> > + cdev->brightness_set_blocking = phy_led_set_brightness;
>
> Please move this initialization to the patch where you are actually
> implementing this callback.
>
> > + cdev->max_brightness = 1;
> > + init_data.devicename = dev_name(&phydev->mdio.dev);
> > + init_data.fwnode = of_fwnode_handle(led);
> > +
> > + err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> > + if (err)
> > + return err;
>
> Another memory leak.
Ah, maybe you don't know about devm_ ? devm_ allocations and actions
register an action to be taken when the device is removed, either
because the probe failed, or when the device is unregistered. For
memory allocation, the memory is freed automagically. For actions like
registering an LED, requesting an interrupt etc, an unregister/release
is performed. This makes cleanup less buggy since the core does it.
Andrew
On Fri, Mar 17, 2023 at 09:14:10AM +0100, Marek Beh?n wrote:
> Hello Christian, also Rob Herring, Andrew Lunn and Pavel Machek,
>
> On Fri, 17 Mar 2023 03:31:21 +0100
> Christian Marangi <[email protected]> wrote:
>
> > Add LEDs definition example for qca8k Switch Family to describe how they
> > should be defined for a correct usage.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
> > ---
> > .../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > index 389892592aac..2e9c14af0223 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > @@ -18,6 +18,8 @@ description:
> > PHY it is connected to. In this config, an internal mdio-bus is registered and
> > the MDIO master is used for communication. Mixed external and internal
> > mdio-bus configurations are not supported by the hardware.
> > + Each phy has at least 3 LEDs connected and can be declared
> > + using the standard LEDs structure.
> >
> > properties:
> > compatible:
> > @@ -117,6 +119,7 @@ unevaluatedProperties: false
> > examples:
> > - |
> > #include <dt-bindings/gpio/gpio.h>
> > + #include <dt-bindings/leds/common.h>
> >
> > mdio {
> > #address-cells = <1>;
> > @@ -226,6 +229,27 @@ examples:
> > label = "lan1";
> > phy-mode = "internal";
> > phy-handle = <&internal_phy_port1>;
> > +
> > + leds {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + led@0 {
> > + reg = <0>;
> > + color = <LED_COLOR_ID_WHITE>;
> > + function = LED_FUNCTION_LAN;
> > + function-enumerator = <1>;
> > + default-state = "keep";
> > + };
> > +
> > + led@1 {
> > + reg = <1>;
> > + color = <LED_COLOR_ID_AMBER>;
> > + function = LED_FUNCTION_LAN;
> > + function-enumerator = <1>;
> > + default-state = "keep";
> > + };
> > + };
> > };
>
> I have nothing against this, but I would like to point out the
> existence of the trigger-sources DT property, and I would like to
> discuss how this property should be used by the LED subsystem to choose
> default behaviour of a LED.
>
> Consider that we want to specify in device-tree that a PHY LED (or any
> other LED) should blink on network activity of the network device
> connected to this PHY (let's say the attached network device is eth0).
> (Why would we want to specify this in devicetree? Because currently the
> drivers either keep the behaviour from boot or change it to something
> specific that is not configurable.)
>
> We could specify in DT something like:
> eth0: ethernet-controller {
> ...
> }
>
> ethernet-phy {
> leds {
> led@0 {
> reg = <0>;
> color = <LED_COLOR_ID_GREEN>;
> trigger-sources = <ð0>;
> function = LED_FUNCTION_ ?????? ;
> }
> }
> }
>
> The above example specifies that the LED has a trigger source (eth0),
> but we still need to specify the trigger itself (for example that
> the LED should blink on activity, or the different kinds of link). In my
> opinion, this should be specified by the function property, but this
> property is currently used in other way: it is filled in with something
> like "wan" or "lan" or "wlan", an information which, IMO,
> should instead come from the devicename part of the LED, not the
> function part.
>
> Recall that the LED names are of the form
> devicename:color:function
> where the devicename part is supposed to be something like mmc0 or
> sda1. With LEDs that are associated with network devices I think the
> corresponding name should be the name of the network device (like eth0),
> but there is the problem of network namespaces and also that network
> devices can be renamed :(.
>
> So one option how to specify the behaviour of the LED to blink on
> activity would be to set
> function = LED_FUNCTION_ACTIVITY;
> but this would conflict with how currently some devicetrees use "lan",
> "wlan" or "wan" as the function (which is IMO incorrect, as I said
> above).
>
> Another option would be to ignore the function and instead use
> additional argument in the trigger-source property, something like
> trigger-sources = <ð0 TRIGGER_SOURCE_ACTIVITY>;
>
> I would like to start a discussion on this and hear about your opinions,
> because I think that the trigger-sources and function properties were
> proposed in good faith, but currently the implementation and usage is a
> mess.
>
I think we should continue and make this discussion when we start
implementing the hw contro for these LEDs to configure them in DT.
Currently we are implementing very basic support so everything will be
in sw.
Anyway just to give some ideas. Yes it sound a good idea to use the
trigger-sources binding. My idea would be that trigger needs to have
specific support for them.
If this in mind netdev can be configured in DT and setup hw control to
offload blink with the required interface passed.
The current implementation still didn't include a way to configure the
blink in DT as the series are already a bit big... (currently we have 3:
- This series that already grow from 10 patch to 14
- A cleanup series for netdev trigger that is already 7 patch
- hw control that is another big boy with 12 patch
)
So our idea was to first implement the minor things and then polish and
improve it. (to make it easier to review)
But agree with you that it would be a nice idea to have a correct and
good implementation for trigger-sources.
--
Ansuel
> We could specify in DT something like:
> eth0: ethernet-controller {
> ...
> }
>
> ethernet-phy {
> leds {
> led@0 {
> reg = <0>;
> color = <LED_COLOR_ID_GREEN>;
> trigger-sources = <ð0>;
> function = LED_FUNCTION_ ?????? ;
> }
> }
> }
For generic case, where you just have an LED on some random bus, you
need to know what netdev it should represent. And in effect, this
patch series gives you just that.
However, when we get to offload, which is the ultimate goal, things
are different. We cannot expect an LED inside the PHY connected to
eth42 to offload blinking for eth24. There is a clear hardware
relationship between the LED and the netdev. And in the more general
case, there will always be a hardware relationship, be it wifi
activity, or hard disk activity. phylib knows this relationship, and
the DSA core also knows this relationship. Probably the SATA core will
know the relationship. So i have a patchset which adds an op to the
cdev to ask it, what struct device do you HW blink for?
trigger-sources then becomes optional. And in fact, if it is used, you
need to verify if it fits to this relationship, and if not, refuse to
offload blinking, so software blinking only.
Anyway, that is an aside to your main question. But the Day Job is
calling. I will address your question later today.
Andrew
On Fri, 17 Mar 2023 14:55:11 +0100
Andrew Lunn <[email protected]> wrote:
> On Fri, Mar 17, 2023 at 08:45:19AM +0100, Marek Behún wrote:
> > On Fri, 17 Mar 2023 03:31:15 +0100
> > Christian Marangi <[email protected]> wrote:
> >
> > > + cdev->brightness_set_blocking = phy_led_set_brightness;
> > > + cdev->max_brightness = 1;
> > > + init_data.devicename = dev_name(&phydev->mdio.dev);
> > > + init_data.fwnode = of_fwnode_handle(led);
> > > +
> > > + err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> >
> > Since init_data.devname_mandatory is false, devicename is ignored.
> > Which is probably good, becuse the device name of a mdio device is
> > often ugly, taken from devicetree or switch drivers, for example:
> > f1072004.mdio-mii
> > fixed-0
> > mv88e6xxx-1
> > So either don't fill devicename or use devname_mandatory (and maybe
> > fill devicename with something less ugly, but I guess if we don't have
> > much choice if we want to keep persistent names).
> >
> > Without devname_mandatory, the name of the LED classdev will be of the
> > form
> > color:function[-function-enumerator],
> > i.e.
> > green:lan
> > amber:lan-1
> >
> > With multiple switch ethenret ports all having LAN function, it is
> > worth noting that the function enumerator must be explicitly used in the
> > devicetree, otherwise multiple LEDs will be registered under the same
> > name, and the LED subsystem will add a number at the and of the name
> > (function led_classdev_next_name), resulting in names
> > green:lan
> > green:lan_1
> > green:lan_2
> > ...
>
> I'm testing on a Marvell RDK, with limited LEDs. It has one LED on the
> front port to represent the WAN port. The DT patch is at the end of
> the series. With that, i end up with:
>
> root@370rd:/sys/class/leds# ls -l
> total 0
> lrwxrwxrwx 1 root root 0 Mar 17 01:10 f1072004.mdio-mii:00:WAN -> ../../devices/platform/soc/soc:interna
> l-regs/f1072004.mdio/mdio_bus/f1072004.mdio-mii/f1072004.mdio-mii:00/leds/f1072004.mdio-mii:00:WAN
>
> I also have:
>
> root@370rd:/sys/class/net/eth0/phydev/leds# ls
> f1072004.mdio-mii:00:WAN
Hmm, yes I see. If label is specified, devicename is used even if
devname_mandatory is false.
> f1072004.mdio-mii:00: is not nice, but it is unique to a netdev. The
> last part then comes from the label property. Since there is only one
> LED, i went with what the port is intended to be used as. If there had
> been more LEDs, i would of probably used labels like "LINK" and
> "ACTIVITY", since that is often what they reset default
> to. Alternatively, you could names the "Left" and "Right", which does
> suggest they can be given any function.
>
> I don't actually think the name is too important, so long as it is
> unique. You are going to find it via /sys/class/net. MAC LEDs should
> be /sys/class/net/eth42/leds, and PHY LEDs will be
> /sys/class/net/phydev/leds.
Maybe the name may not be that important from the perspective of a user
who just wants to find the LED for a given phy, yes, but the
proposal of how LED classdev should be named was done in good faith
and accepted years ago. The documentation still defines the name format
and until that part of documenation is changed, I think we should at
least try to follow it.
Also, the label DT property has been deprecated for LEDs. IMO it should
be removed from that last patch of this series.
> It has been discussed in the past to either extend ethtool to
> understand this, or write a new little tool to make it easier to
> manipulate these LEDs.
Yes, and this would solve the problem for a user who wants to change
the behaviour of a LED for a given PHY. But a user who wants to list
all available LEDs by listing /sys/class/leds can also retrieve a nice
list of names that make sense, if the documented format is followed.
Marek
On Fri, Mar 17, 2023 at 03:03:32PM +0100, Andrew Lunn wrote:
> > Hi Andrew,
> >
> > Personally, I see no good reason to provide a dummy implementation
> > of "phy_led_set_brightness", especially if you implement it in the next
> > patch. You only use that function only the function pointer in
> > "led_classdev". I think you can just skip it in this patch.
>
> Hi Michal
>
> The basic code for this patch has been sitting in my tree for a long
> time. It used to be, if you did not have a set_brightness method in
> cdev, the registration failed. That made it hard to test this patch on
> its own during development work, did i have the link list correct, can
> i unload the PHY driver without it exploding etc. I need to check if
> it is still mandatory.
>
Thank you for the explanation. I was not aware of failing registration
in case of undefined "cdev->brightness_set_blocking". I think it is
a good reason of defining the dummy function. (The only alternative
would be to squash two commits, but I think it is easier to review
smaller chunks of code).
> > > +static int of_phy_led(struct phy_device *phydev,
> > > + struct device_node *led)
> > > +{
> > > + struct device *dev = &phydev->mdio.dev;
> > > + struct led_init_data init_data = {};
> > > + struct led_classdev *cdev;
> > > + struct phy_led *phyled;
> > > + int err;
> > > +
> > > + phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
> > > + if (!phyled)
> > > + return -ENOMEM;
> > > +
> > > + cdev = &phyled->led_cdev;
> > > +
> > > + err = of_property_read_u32(led, "reg", &phyled->index);
> > > + if (err)
> > > + return err;
> >
> > Memory leak. 'phyled' is not freed in case of error.
>
> devm_ API, so it gets freed when the probe fails.
>
> > > +
> > > + cdev->brightness_set_blocking = phy_led_set_brightness;
> >
> > Please move this initialization to the patch where you are actually
> > implementing this callback.
> >
> > > + cdev->max_brightness = 1;
> > > + init_data.devicename = dev_name(&phydev->mdio.dev);
> > > + init_data.fwnode = of_fwnode_handle(led);
> > > +
> > > + err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> > > + if (err)
> > > + return err;
> >
> > Another memory leak.
>
> Ah, maybe you don't know about devm_ ? devm_ allocations and actions
> register an action to be taken when the device is removed, either
> because the probe failed, or when the device is unregistered. For
> memory allocation, the memory is freed automagically. For actions like
> registering an LED, requesting an interrupt etc, an unregister/release
> is performed. This makes cleanup less buggy since the core does it.
>
> Andrew
Yeah, it is my fault, I apologize for that.
I didn't consider neither the probe() context, nor the lifetime of the
list. You are right - I had no experience with using this devm_ API,
so I looked at it as a standard memory allocation.
Thank you for your patience and this piece of knowledge.
Thanks,
Michal
> Yes, and this would solve the problem for a user who wants to change
> the behaviour of a LED for a given PHY. But a user who wants to list
> all available LEDs by listing /sys/class/leds can also retrieve a nice
> list of names that make sense, if the documented format is followed.
Please make a concrete proposal.
Also, keep in mind, ethernet device names change at runtime, and are
not unique. Function also changes at run time, which is in fact the
whole purpose of this collection of patchsets.
Andrew
> I would like to start a discussion on this and hear about your opinions,
> because I think that the trigger-sources and function properties were
> proposed in good faith, but currently the implementation and usage is a
> mess.
Hi Marek
We are pushing the boundaries of the LED code here, doing things which
have not been done before, as far as i know. So i expect some
discussion about this. However, that discussion should not really
affect this patchset, which is just adding plain boring software
controlled LEDs.
A quick recap about ledtrig-netdev.
If you have a plain boring LED, you have:
root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# ls
brightness device max_brightness power subsystem trigger uevent
You can turn the LED on with
root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo 1 > brightness
and turn it off with:
root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo 0 > brightness
You select the trigger via the trigger sysfs file:
root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# cat trigger
[none] kbd-scrolllock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock timer heartbeat netdev mmc0
root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo netdev > trigger
root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# ls
activity brightness device_name half_duplex link link_100 max_brightness rx trigger uevent
available_mode device full_duplex interval link_10 link_1000 power subsystem tx
When you select a trigger, that trigger can add additional sysfs
files. For the netdev trigger we gain link, link_10, link_100, link_1000, rx & tx.
Nothing special here, if you selected the timer trigger you get
delay_off delay_on. The oneshot trigger has invert, delay_on,
delay_off etc.
You then configure the trigger via setting values in the sysfs
files. If you want the LED to indicate if there is link, you would do:
echo 1 > link
The LED would then light up if the netdev has carrier.
If you want link plus RX packets
echo 1 > link
echo 1 > rx
The LED will then be on if there is link, and additionally blink if
the netdev stats indicate received frames.
For the netdev trigger, all the configuration values are boolean. So a
simple way to represent this in DT would be boolean properties:
netdev-link = <1>;
netdev->rx = <1>;
We probably want these properties name spaced, because we have oneshot
delay_on and timer delay_on for example. The same sysfs name could
have different types, bool vs milliseconds, etc.
I would make it, that when the trigger is activated, the values are
read from DT and used. There is currently no persistent state for
triggers. If you where to swap to the timer trigger and then return to
the netdev trigger, all state is lost, so i would re-read DT.
Offloading to hardware should not make an difference here. All we are
going to do is pass the current configuration to the LED and ask it,
can you do this? If it says no, we keep blinking in software. If yes,
we leave the blinking to the hardware.
There is the open question of if DT should be used like this. It is
not describing hardware, it is describing configuration of
hardware. So it could well get rejected. You then need to configure it
in software.
Andrew
On Fri, Mar 17, 2023 at 03:01:28PM +0100, Christian Marangi wrote:
> On Fri, Mar 17, 2023 at 12:24:23PM +0100, Michal Kubiak wrote:
> > On Fri, Mar 17, 2023 at 03:31:13AM +0100, Christian Marangi wrote:
> > > Add LEDs basic support for qca8k Switch Family by adding basic
> > > brightness_set() support.
> > >
> > > Since these LEDs refelect port status, the default label is set to
> > > ":port". DT binding should describe the color, function and number of
> > > the leds using standard LEDs api.
> > >
> > > These LEDs supports only blocking variant of the brightness_set()
> > > function since they can sleep during access of the switch leds to set
> > > the brightness.
> > >
> > > While at it add to the qca8k header file each mode defined by the Switch
> > > Documentation for future use.
> > >
> > > Signed-off-by: Christian Marangi <[email protected]>
> >
> > Hi Christian,
> >
> > Please find my comments inline.
> >
> > Thanks,
> > Michal
> >
> > > ---
> > > drivers/net/dsa/qca/Kconfig | 8 ++
> > > drivers/net/dsa/qca/Makefile | 3 +
> > > drivers/net/dsa/qca/qca8k-8xxx.c | 5 +
> > > drivers/net/dsa/qca/qca8k-leds.c | 192 +++++++++++++++++++++++++++++++
> > > drivers/net/dsa/qca/qca8k.h | 59 ++++++++++
> > > drivers/net/dsa/qca/qca8k_leds.h | 16 +++
> > > 6 files changed, 283 insertions(+)
> > > create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
> > > create mode 100644 drivers/net/dsa/qca/qca8k_leds.h
> > >
> > > diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
> > > index ba339747362c..7a86d6d6a246 100644
> > > --- a/drivers/net/dsa/qca/Kconfig
> > > +++ b/drivers/net/dsa/qca/Kconfig
> > > @@ -15,3 +15,11 @@ config NET_DSA_QCA8K
> > > help
> > > This enables support for the Qualcomm Atheros QCA8K Ethernet
> > > switch chips.
> > > +
> > > +config NET_DSA_QCA8K_LEDS_SUPPORT
> > > + bool "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
> > > + depends on NET_DSA_QCA8K
> > > + depends on LEDS_CLASS
> > > + help
> > > + This enabled support for LEDs present on the Qualcomm Atheros
> > > + QCA8K Ethernet switch chips.
> > > diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
> > > index 701f1d199e93..ce66b1984e5f 100644
> > > --- a/drivers/net/dsa/qca/Makefile
> > > +++ b/drivers/net/dsa/qca/Makefile
> > > @@ -2,3 +2,6 @@
> > > obj-$(CONFIG_NET_DSA_AR9331) += ar9331.o
> > > obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
> > > qca8k-y += qca8k-common.o qca8k-8xxx.o
> > > +ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
> > > +qca8k-y += qca8k-leds.o
> > > +endif
> > > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> > > index 8dfc5db84700..5decf6fe3832 100644
> > > --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> > > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> > > @@ -22,6 +22,7 @@
> > > #include <linux/dsa/tag_qca.h>
> > >
> > > #include "qca8k.h"
> > > +#include "qca8k_leds.h"
> > >
> > > static void
> > > qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
> > > @@ -1727,6 +1728,10 @@ qca8k_setup(struct dsa_switch *ds)
> > > if (ret)
> > > return ret;
> > >
> > > + ret = qca8k_setup_led_ctrl(priv);
> > > + if (ret)
> > > + return ret;
> > > +
> > > qca8k_setup_pcs(priv, &priv->pcs_port_0, 0);
> > > qca8k_setup_pcs(priv, &priv->pcs_port_6, 6);
> > >
> > > diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
> > > new file mode 100644
> > > index 000000000000..adbe7f6e2994
> > > --- /dev/null
> > > +++ b/drivers/net/dsa/qca/qca8k-leds.c
> > > @@ -0,0 +1,192 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include <linux/regmap.h>
> > > +#include <net/dsa.h>
> > > +
> > > +#include "qca8k.h"
> > > +#include "qca8k_leds.h"
> > > +
> > > +static int
> > > +qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
> > > +{
> > > + switch (port_num) {
> > > + case 0:
> > > + reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> > > + reg_info->shift = QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT;
> > > + break;
> > > + case 1:
> > > + case 2:
> > > + case 3:
> > > + /* Port 123 are controlled on a different reg */
> > > + reg_info->reg = QCA8K_LED_CTRL_REG(3);
> > > + reg_info->shift = QCA8K_LED_PHY123_PATTERN_EN_SHIFT(port_num, led_num);
> > > + break;
> > > + case 4:
> > > + reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> > > + reg_info->shift = QCA8K_LED_PHY4_CONTROL_RULE_SHIFT;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int
> > > +qca8k_led_brightness_set(struct qca8k_led *led,
> > > + enum led_brightness brightness)
> > > +{
> > > + struct qca8k_led_pattern_en reg_info;
> > > + struct qca8k_priv *priv = led->priv;
> > > + u32 mask, val = QCA8K_LED_ALWAYS_OFF;
> >
> > Nitpick: RCT
> >
> > > +
> > > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > > +
> > > + if (brightness)
> > > + val = QCA8K_LED_ALWAYS_ON;
> > > +
> > > + if (led->port_num == 0 || led->port_num == 4) {
> > > + mask = QCA8K_LED_PATTERN_EN_MASK;
> > > + val <<= QCA8K_LED_PATTERN_EN_SHIFT;
> > > + } else {
> > > + mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
> > > + }
> > > +
> > > + return regmap_update_bits(priv->regmap, reg_info.reg,
> > > + mask << reg_info.shift,
> > > + val << reg_info.shift);
> > > +}
> > > +
> > > +static int
> > > +qca8k_cled_brightness_set_blocking(struct led_classdev *ldev,
> > > + enum led_brightness brightness)
> > > +{
> > > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > > +
> > > + return qca8k_led_brightness_set(led, brightness);
> > > +}
> > > +
> > > +static enum led_brightness
> > > +qca8k_led_brightness_get(struct qca8k_led *led)
> > > +{
> > > + struct qca8k_led_pattern_en reg_info;
> > > + struct qca8k_priv *priv = led->priv;
> > > + u32 val;
> > > + int ret;
> > > +
> > > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > > +
> > > + ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > > + if (ret)
> > > + return 0;
> > > +
> > > + val >>= reg_info.shift;
> > > +
> > > + if (led->port_num == 0 || led->port_num == 4) {
> > > + val &= QCA8K_LED_PATTERN_EN_MASK;
> > > + val >>= QCA8K_LED_PATTERN_EN_SHIFT;
> > > + } else {
> > > + val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
> > > + }
> > > +
> > > + /* Assume brightness ON only when the LED is set to always ON */
> > > + return val == QCA8K_LED_ALWAYS_ON;
> > > +}
> > > +
> > > +static int
> > > +qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
> > > +{
> > > + struct fwnode_handle *led = NULL, *leds = NULL;
> > > + struct led_init_data init_data = { };
> > > + enum led_default_state state;
> > > + struct qca8k_led *port_led;
> > > + int led_num, port_index;
> > > + int ret;
> > > +
> > > + leds = fwnode_get_named_child_node(port, "leds");
> > > + if (!leds) {
> > > + dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
> > > + port_num);
> > > + return 0;
> > > + }
> > > +
> > > + fwnode_for_each_child_node(leds, led) {
> > > + /* Reg represent the led number of the port.
> > > + * Each port can have at least 3 leds attached
> > > + * Commonly:
> > > + * 1. is gigabit led
> > > + * 2. is mbit led
> > > + * 3. additional status led
> > > + */
> > > + if (fwnode_property_read_u32(led, "reg", &led_num))
> > > + continue;
> > > +
> > > + if (led_num >= QCA8K_LED_PORT_COUNT) {
> > > + dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
> > > + continue;
> > > + }
> >
> > In the comment above you say "each port can have AT LEAST 3 leds".
> > However, now it seems that if the port has more than 3 leds, all the
> > remaining leds are not initialized.
> > Is this intentional? If so, maybe it is worth describing in the comment
> > that for ports with more than 3 leds, only the first 3 leds are
> > initialized?
> >
> > According to the code it looks like the port can have up to 3 leds.
> >
>
> Think I should rework the comments and make them more direct/simple.
>
> qca8k switch have a max of 5 port.
>
> each port CAN have a max of 3 leds connected.
>
> It's really a limitation of pin on the switch chip and hw regs so the
> situation can't happen.
OK, so it looks like a misunderstanding.
I interpreted the sentence:
"each port can have AT LEAST 3 leds"
as
"each port can a MIN of 3 leds connected".
Most likely it is just a typo and it is a matter of changing "at
least" to "at most" in the comment :-).
>
> > > +
> > > + port_index = 3 * port_num + led_num;
> >
> > Can QCA8K_LED_PORT_COUNT be used instead of "3"? I guess it is the number
> > of LEDs per port.
> >
>
> This variable it's really to make it easier to reference the led in the
> priv struct. If asked I can rework this to an array of array (one per
> port and each port out of 3 possigle LED).
I wasn't probably clear. I just wanted to ask if you can use the constant
"QCA8K_LED_PORT_COUNT" instead of a raw number "3".
BTW, I agree that "array of array" option seems too complex solution for
a simple thing :-).
>
> > > +
> > > + port_led = &priv->ports_led[port_index];
> >
> > Also, the name of the "port_index" variable seems confusing to me. It is
> > not an index of the port, but rather a unique index of the LED across
> > all ports, right?
> >
>
> As said above, they are unique index that comes from port and LED of the
> port. Really something to represent the code easier internally.
Got it. I was just sharing my impression of the name "port_index" itself.
Maybe, "led_index" would be better because it actually indexes leds, not
ports?
>
> > > + port_led->port_num = port_num;
> > > + port_led->led_num = led_num;
> > > + port_led->priv = priv;
> > > +
> > > + state = led_init_default_state_get(led);
> > > + switch (state) {
> > > + case LEDS_DEFSTATE_ON:
> > > + port_led->cdev.brightness = 1;
> > > + qca8k_led_brightness_set(port_led, 1);
> > > + break;
> > > + case LEDS_DEFSTATE_KEEP:
> > > + port_led->cdev.brightness =
> > > + qca8k_led_brightness_get(port_led);
> > > + break;
> > > + default:
> > > + port_led->cdev.brightness = 0;
> > > + qca8k_led_brightness_set(port_led, 0);
> > > + }
> > > +
> > > + port_led->cdev.max_brightness = 1;
> > > + port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
> > > + init_data.default_label = ":port";
> > > + init_data.devicename = "qca8k";
> > > + init_data.fwnode = led;
> > > +
> > > + ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
> > > + if (ret)
> > > + dev_warn(priv->dev, "Failed to int led");
> >
> > Typo: "init".
> > How about adding an index of the LED that could not be initialized?
> >
>
> Ok will add more info in the port and led that failed to init.
Thanks, but it's just my suggestion.
>
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int
> > > +qca8k_setup_led_ctrl(struct qca8k_priv *priv)
> > > +{
> > > + struct fwnode_handle *ports, *port;
> > > + int port_num;
> > > + int ret;
> > > +
> > > + ports = device_get_named_child_node(priv->dev, "ports");
> > > + if (!ports) {
> > > + dev_info(priv->dev, "No ports node specified in device tree!\n");
> > > + return 0;
> > > + }
> > > +
> > > + fwnode_for_each_child_node(ports, port) {
> > > + if (fwnode_property_read_u32(port, "reg", &port_num))
> > > + continue;
> > > +
> > > + /* Each port can have at least 3 different leds attached.
"at least" -> "at most"
> > > + * Switch port starts from 0 to 6, but port 0 and 6 are CPU
> > > + * port. The port index needs to be decreased by one to identify
> > > + * the correct port for LED setup.
> > > + */
> >
> > Again, are there really "at least 3 different leds" per port?
> > It's confusing a little bit, because QCA8K_LED_PORT_COUNT == 3, so I
> > would say it cannot have more than 3.
> >
> > > + ret = qca8k_parse_port_leds(priv, port, qca8k_port_to_phy(port_num));
> >
> > As I checked, the function "qca8k_port_to_phy()" can return all 0xFFs
> > for port_num == 0. Then, this value is implicitly casted to int (as the
> > last parameter of "qca8k_parse_port_leds()"). Internally, in
> > "qca8k_parse_port_leds()" this parameter can be used to do some
> > computing - that looks dangerous.
> > In summary, I think a special check for CPU port_num == 0 should be
> > added.
> > (I guess the LED configuration i only makes sense for non-CPU ports? It
> > seems you want to configure up to 15 LEDs in total for 5 ports).
> >
>
> IMHO for this, we can ignore handling this corner case. The hw doesn't
> supports leds for port0 and port6 (the 2 CPU port) so the case won't
> ever apply. But if asked I can add the case, not that it will cause any
> problem in how the regs and shift are referenced in the code.
OK, got it. So, as I understand, the previous call in this loop
"fwnode_property_read_u32()" will never return port_num == 0 (or we fall
into "continue")?
>
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> > > index 4e48e4dd8b0f..3c3c072fa9c2 100644
> > > --- a/drivers/net/dsa/qca/qca8k.h
> > > +++ b/drivers/net/dsa/qca/qca8k.h
> > > @@ -11,6 +11,7 @@
> > > #include <linux/delay.h>
> > > #include <linux/regmap.h>
> > > #include <linux/gpio.h>
> > > +#include <linux/leds.h>
> > > #include <linux/dsa/tag_qca.h>
> > >
> > > #define QCA8K_ETHERNET_MDIO_PRIORITY 7
> > > @@ -85,6 +86,50 @@
> > > #define QCA8K_MDIO_MASTER_DATA(x) FIELD_PREP(QCA8K_MDIO_MASTER_DATA_MASK, x)
> > > #define QCA8K_MDIO_MASTER_MAX_PORTS 5
> > > #define QCA8K_MDIO_MASTER_MAX_REG 32
> > > +
> > > +/* LED control register */
> > > +#define QCA8K_LED_COUNT 15
> > > +#define QCA8K_LED_PORT_COUNT 3
> > > +#define QCA8K_LED_RULE_COUNT 6
> > > +#define QCA8K_LED_RULE_MAX 11
> > > +
> > > +#define QCA8K_LED_PHY123_PATTERN_EN_SHIFT(_phy, _led) ((((_phy) - 1) * 6) + 8 + (2 * (_led)))
> > > +#define QCA8K_LED_PHY123_PATTERN_EN_MASK GENMASK(1, 0)
> > > +
> > > +#define QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT 0
> > > +#define QCA8K_LED_PHY4_CONTROL_RULE_SHIFT 16
> > > +
> > > +#define QCA8K_LED_CTRL_REG(_i) (0x050 + (_i) * 4)
> > > +#define QCA8K_LED_CTRL0_REG 0x50
> > > +#define QCA8K_LED_CTRL1_REG 0x54
> > > +#define QCA8K_LED_CTRL2_REG 0x58
> > > +#define QCA8K_LED_CTRL3_REG 0x5C
> > > +#define QCA8K_LED_CTRL_SHIFT(_i) (((_i) % 2) * 16)
> > > +#define QCA8K_LED_CTRL_MASK GENMASK(15, 0)
> > > +#define QCA8K_LED_RULE_MASK GENMASK(13, 0)
> > > +#define QCA8K_LED_BLINK_FREQ_MASK GENMASK(1, 0)
> > > +#define QCA8K_LED_BLINK_FREQ_SHITF 0
> > > +#define QCA8K_LED_BLINK_2HZ 0
> > > +#define QCA8K_LED_BLINK_4HZ 1
> > > +#define QCA8K_LED_BLINK_8HZ 2
> > > +#define QCA8K_LED_BLINK_AUTO 3
> > > +#define QCA8K_LED_LINKUP_OVER_MASK BIT(2)
> > > +#define QCA8K_LED_TX_BLINK_MASK BIT(4)
> > > +#define QCA8K_LED_RX_BLINK_MASK BIT(5)
> > > +#define QCA8K_LED_COL_BLINK_MASK BIT(7)
> > > +#define QCA8K_LED_LINK_10M_EN_MASK BIT(8)
> > > +#define QCA8K_LED_LINK_100M_EN_MASK BIT(9)
> > > +#define QCA8K_LED_LINK_1000M_EN_MASK BIT(10)
> > > +#define QCA8K_LED_POWER_ON_LIGHT_MASK BIT(11)
> > > +#define QCA8K_LED_HALF_DUPLEX_MASK BIT(12)
> > > +#define QCA8K_LED_FULL_DUPLEX_MASK BIT(13)
> > > +#define QCA8K_LED_PATTERN_EN_MASK GENMASK(15, 14)
> > > +#define QCA8K_LED_PATTERN_EN_SHIFT 14
> > > +#define QCA8K_LED_ALWAYS_OFF 0
> > > +#define QCA8K_LED_ALWAYS_BLINK_4HZ 1
> > > +#define QCA8K_LED_ALWAYS_ON 2
> > > +#define QCA8K_LED_RULE_CONTROLLED 3
> > > +
> > > #define QCA8K_GOL_MAC_ADDR0 0x60
> > > #define QCA8K_GOL_MAC_ADDR1 0x64
> > > #define QCA8K_MAX_FRAME_SIZE 0x78
> > > @@ -383,6 +428,19 @@ struct qca8k_pcs {
> > > int port;
> > > };
> > >
> > > +struct qca8k_led_pattern_en {
> > > + u32 reg;
> > > + u8 shift;
> > > +};
> > > +
> > > +struct qca8k_led {
> > > + u8 port_num;
> > > + u8 led_num;
> > > + u16 old_rule;
> > > + struct qca8k_priv *priv;
> > > + struct led_classdev cdev;
> > > +};
> > > +
> > > struct qca8k_priv {
> > > u8 switch_id;
> > > u8 switch_revision;
> > > @@ -407,6 +465,7 @@ struct qca8k_priv {
> > > struct qca8k_pcs pcs_port_0;
> > > struct qca8k_pcs pcs_port_6;
> > > const struct qca8k_match_data *info;
> > > + struct qca8k_led ports_led[QCA8K_LED_COUNT];
> > > };
> > >
> > > struct qca8k_mib_desc {
> > > diff --git a/drivers/net/dsa/qca/qca8k_leds.h b/drivers/net/dsa/qca/qca8k_leds.h
> > > new file mode 100644
> > > index 000000000000..ab367f05b173
> > > --- /dev/null
> > > +++ b/drivers/net/dsa/qca/qca8k_leds.h
> > > @@ -0,0 +1,16 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +
> > > +#ifndef __QCA8K_LEDS_H
> > > +#define __QCA8K_LEDS_H
> > > +
> > > +/* Leds Support function */
> > > +#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
> > > +int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
> > > +#else
> > > +static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
> > > +{
> > > + return 0;
> > > +}
> > > +#endif
> > > +
> > > +#endif /* __QCA8K_LEDS_H */
> > > --
> > > 2.39.2
> > >
>
> --
> Ansuel
Thanks,
Michal
On Fri, Mar 17, 2023 at 07:05:57PM +0100, Michal Kubiak wrote:
> On Fri, Mar 17, 2023 at 03:01:28PM +0100, Christian Marangi wrote:
> > On Fri, Mar 17, 2023 at 12:24:23PM +0100, Michal Kubiak wrote:
> > > On Fri, Mar 17, 2023 at 03:31:13AM +0100, Christian Marangi wrote:
> > > > Add LEDs basic support for qca8k Switch Family by adding basic
> > > > brightness_set() support.
> > > >
> > > > Since these LEDs refelect port status, the default label is set to
> > > > ":port". DT binding should describe the color, function and number of
> > > > the leds using standard LEDs api.
> > > >
> > > > These LEDs supports only blocking variant of the brightness_set()
> > > > function since they can sleep during access of the switch leds to set
> > > > the brightness.
> > > >
> > > > While at it add to the qca8k header file each mode defined by the Switch
> > > > Documentation for future use.
> > > >
> > > > Signed-off-by: Christian Marangi <[email protected]>
> > >
> > > Hi Christian,
> > >
> > > Please find my comments inline.
> > >
> > > Thanks,
> > > Michal
> > >
> > > > ---
> > > > drivers/net/dsa/qca/Kconfig | 8 ++
> > > > drivers/net/dsa/qca/Makefile | 3 +
> > > > drivers/net/dsa/qca/qca8k-8xxx.c | 5 +
> > > > drivers/net/dsa/qca/qca8k-leds.c | 192 +++++++++++++++++++++++++++++++
> > > > drivers/net/dsa/qca/qca8k.h | 59 ++++++++++
> > > > drivers/net/dsa/qca/qca8k_leds.h | 16 +++
> > > > 6 files changed, 283 insertions(+)
> > > > create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
> > > > create mode 100644 drivers/net/dsa/qca/qca8k_leds.h
> > > >
> > > > diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
> > > > index ba339747362c..7a86d6d6a246 100644
> > > > --- a/drivers/net/dsa/qca/Kconfig
> > > > +++ b/drivers/net/dsa/qca/Kconfig
> > > > @@ -15,3 +15,11 @@ config NET_DSA_QCA8K
> > > > help
> > > > This enables support for the Qualcomm Atheros QCA8K Ethernet
> > > > switch chips.
> > > > +
> > > > +config NET_DSA_QCA8K_LEDS_SUPPORT
> > > > + bool "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
> > > > + depends on NET_DSA_QCA8K
> > > > + depends on LEDS_CLASS
> > > > + help
> > > > + This enabled support for LEDs present on the Qualcomm Atheros
> > > > + QCA8K Ethernet switch chips.
> > > > diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
> > > > index 701f1d199e93..ce66b1984e5f 100644
> > > > --- a/drivers/net/dsa/qca/Makefile
> > > > +++ b/drivers/net/dsa/qca/Makefile
> > > > @@ -2,3 +2,6 @@
> > > > obj-$(CONFIG_NET_DSA_AR9331) += ar9331.o
> > > > obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
> > > > qca8k-y += qca8k-common.o qca8k-8xxx.o
> > > > +ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
> > > > +qca8k-y += qca8k-leds.o
> > > > +endif
> > > > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> > > > index 8dfc5db84700..5decf6fe3832 100644
> > > > --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> > > > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> > > > @@ -22,6 +22,7 @@
> > > > #include <linux/dsa/tag_qca.h>
> > > >
> > > > #include "qca8k.h"
> > > > +#include "qca8k_leds.h"
> > > >
> > > > static void
> > > > qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
> > > > @@ -1727,6 +1728,10 @@ qca8k_setup(struct dsa_switch *ds)
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + ret = qca8k_setup_led_ctrl(priv);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > qca8k_setup_pcs(priv, &priv->pcs_port_0, 0);
> > > > qca8k_setup_pcs(priv, &priv->pcs_port_6, 6);
> > > >
> > > > diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
> > > > new file mode 100644
> > > > index 000000000000..adbe7f6e2994
> > > > --- /dev/null
> > > > +++ b/drivers/net/dsa/qca/qca8k-leds.c
> > > > @@ -0,0 +1,192 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +#include <linux/regmap.h>
> > > > +#include <net/dsa.h>
> > > > +
> > > > +#include "qca8k.h"
> > > > +#include "qca8k_leds.h"
> > > > +
> > > > +static int
> > > > +qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
> > > > +{
> > > > + switch (port_num) {
> > > > + case 0:
> > > > + reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> > > > + reg_info->shift = QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT;
> > > > + break;
> > > > + case 1:
> > > > + case 2:
> > > > + case 3:
> > > > + /* Port 123 are controlled on a different reg */
> > > > + reg_info->reg = QCA8K_LED_CTRL_REG(3);
> > > > + reg_info->shift = QCA8K_LED_PHY123_PATTERN_EN_SHIFT(port_num, led_num);
> > > > + break;
> > > > + case 4:
> > > > + reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> > > > + reg_info->shift = QCA8K_LED_PHY4_CONTROL_RULE_SHIFT;
> > > > + break;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +qca8k_led_brightness_set(struct qca8k_led *led,
> > > > + enum led_brightness brightness)
> > > > +{
> > > > + struct qca8k_led_pattern_en reg_info;
> > > > + struct qca8k_priv *priv = led->priv;
> > > > + u32 mask, val = QCA8K_LED_ALWAYS_OFF;
> > >
> > > Nitpick: RCT
> > >
> > > > +
> > > > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > > > +
> > > > + if (brightness)
> > > > + val = QCA8K_LED_ALWAYS_ON;
> > > > +
> > > > + if (led->port_num == 0 || led->port_num == 4) {
> > > > + mask = QCA8K_LED_PATTERN_EN_MASK;
> > > > + val <<= QCA8K_LED_PATTERN_EN_SHIFT;
> > > > + } else {
> > > > + mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
> > > > + }
> > > > +
> > > > + return regmap_update_bits(priv->regmap, reg_info.reg,
> > > > + mask << reg_info.shift,
> > > > + val << reg_info.shift);
> > > > +}
> > > > +
> > > > +static int
> > > > +qca8k_cled_brightness_set_blocking(struct led_classdev *ldev,
> > > > + enum led_brightness brightness)
> > > > +{
> > > > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > > > +
> > > > + return qca8k_led_brightness_set(led, brightness);
> > > > +}
> > > > +
> > > > +static enum led_brightness
> > > > +qca8k_led_brightness_get(struct qca8k_led *led)
> > > > +{
> > > > + struct qca8k_led_pattern_en reg_info;
> > > > + struct qca8k_priv *priv = led->priv;
> > > > + u32 val;
> > > > + int ret;
> > > > +
> > > > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > > > +
> > > > + ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > > > + if (ret)
> > > > + return 0;
> > > > +
> > > > + val >>= reg_info.shift;
> > > > +
> > > > + if (led->port_num == 0 || led->port_num == 4) {
> > > > + val &= QCA8K_LED_PATTERN_EN_MASK;
> > > > + val >>= QCA8K_LED_PATTERN_EN_SHIFT;
> > > > + } else {
> > > > + val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
> > > > + }
> > > > +
> > > > + /* Assume brightness ON only when the LED is set to always ON */
> > > > + return val == QCA8K_LED_ALWAYS_ON;
> > > > +}
> > > > +
> > > > +static int
> > > > +qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
> > > > +{
> > > > + struct fwnode_handle *led = NULL, *leds = NULL;
> > > > + struct led_init_data init_data = { };
> > > > + enum led_default_state state;
> > > > + struct qca8k_led *port_led;
> > > > + int led_num, port_index;
> > > > + int ret;
> > > > +
> > > > + leds = fwnode_get_named_child_node(port, "leds");
> > > > + if (!leds) {
> > > > + dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
> > > > + port_num);
> > > > + return 0;
> > > > + }
> > > > +
> > > > + fwnode_for_each_child_node(leds, led) {
> > > > + /* Reg represent the led number of the port.
> > > > + * Each port can have at least 3 leds attached
> > > > + * Commonly:
> > > > + * 1. is gigabit led
> > > > + * 2. is mbit led
> > > > + * 3. additional status led
> > > > + */
> > > > + if (fwnode_property_read_u32(led, "reg", &led_num))
> > > > + continue;
> > > > +
> > > > + if (led_num >= QCA8K_LED_PORT_COUNT) {
> > > > + dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
> > > > + continue;
> > > > + }
> > >
> > > In the comment above you say "each port can have AT LEAST 3 leds".
> > > However, now it seems that if the port has more than 3 leds, all the
> > > remaining leds are not initialized.
> > > Is this intentional? If so, maybe it is worth describing in the comment
> > > that for ports with more than 3 leds, only the first 3 leds are
> > > initialized?
> > >
> > > According to the code it looks like the port can have up to 3 leds.
> > >
> >
> > Think I should rework the comments and make them more direct/simple.
> >
> > qca8k switch have a max of 5 port.
> >
> > each port CAN have a max of 3 leds connected.
> >
> > It's really a limitation of pin on the switch chip and hw regs so the
> > situation can't happen.
>
> OK, so it looks like a misunderstanding.
> I interpreted the sentence:
> "each port can have AT LEAST 3 leds"
> as
> "each port can a MIN of 3 leds connected".
>
> Most likely it is just a typo and it is a matter of changing "at
> least" to "at most" in the comment :-).
>
Yep just that. Thanks a lot for poiting this out.
> >
> > > > +
> > > > + port_index = 3 * port_num + led_num;
> > >
> > > Can QCA8K_LED_PORT_COUNT be used instead of "3"? I guess it is the number
> > > of LEDs per port.
> > >
> >
> > This variable it's really to make it easier to reference the led in the
> > priv struct. If asked I can rework this to an array of array (one per
> > port and each port out of 3 possigle LED).
>
> I wasn't probably clear. I just wanted to ask if you can use the constant
> "QCA8K_LED_PORT_COUNT" instead of a raw number "3".
>
> BTW, I agree that "array of array" option seems too complex solution for
> a simple thing :-).
>
Decided to move the index calculation in the header.
> >
> > > > +
> > > > + port_led = &priv->ports_led[port_index];
> > >
> > > Also, the name of the "port_index" variable seems confusing to me. It is
> > > not an index of the port, but rather a unique index of the LED across
> > > all ports, right?
> > >
> >
> > As said above, they are unique index that comes from port and LED of the
> > port. Really something to represent the code easier internally.
>
> Got it. I was just sharing my impression of the name "port_index" itself.
> Maybe, "led_index" would be better because it actually indexes leds, not
> ports?
>
Funny enough I just started working on this and I just decided the same
name and then I read your suggestion. Anyway yep correct port_index
was very confusing.
> >
> > > > + port_led->port_num = port_num;
> > > > + port_led->led_num = led_num;
> > > > + port_led->priv = priv;
> > > > +
> > > > + state = led_init_default_state_get(led);
> > > > + switch (state) {
> > > > + case LEDS_DEFSTATE_ON:
> > > > + port_led->cdev.brightness = 1;
> > > > + qca8k_led_brightness_set(port_led, 1);
> > > > + break;
> > > > + case LEDS_DEFSTATE_KEEP:
> > > > + port_led->cdev.brightness =
> > > > + qca8k_led_brightness_get(port_led);
> > > > + break;
> > > > + default:
> > > > + port_led->cdev.brightness = 0;
> > > > + qca8k_led_brightness_set(port_led, 0);
> > > > + }
> > > > +
> > > > + port_led->cdev.max_brightness = 1;
> > > > + port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
> > > > + init_data.default_label = ":port";
> > > > + init_data.devicename = "qca8k";
> > > > + init_data.fwnode = led;
> > > > +
> > > > + ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
> > > > + if (ret)
> > > > + dev_warn(priv->dev, "Failed to int led");
> > >
> > > Typo: "init".
> > > How about adding an index of the LED that could not be initialized?
> > >
> >
> > Ok will add more info in the port and led that failed to init.
>
> Thanks, but it's just my suggestion.
>
> >
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int
> > > > +qca8k_setup_led_ctrl(struct qca8k_priv *priv)
> > > > +{
> > > > + struct fwnode_handle *ports, *port;
> > > > + int port_num;
> > > > + int ret;
> > > > +
> > > > + ports = device_get_named_child_node(priv->dev, "ports");
> > > > + if (!ports) {
> > > > + dev_info(priv->dev, "No ports node specified in device tree!\n");
> > > > + return 0;
> > > > + }
> > > > +
> > > > + fwnode_for_each_child_node(ports, port) {
> > > > + if (fwnode_property_read_u32(port, "reg", &port_num))
> > > > + continue;
> > > > +
> > > > + /* Each port can have at least 3 different leds attached.
>
> "at least" -> "at most"
>
> > > > + * Switch port starts from 0 to 6, but port 0 and 6 are CPU
> > > > + * port. The port index needs to be decreased by one to identify
> > > > + * the correct port for LED setup.
> > > > + */
> > >
> > > Again, are there really "at least 3 different leds" per port?
> > > It's confusing a little bit, because QCA8K_LED_PORT_COUNT == 3, so I
> > > would say it cannot have more than 3.
> > >
> > > > + ret = qca8k_parse_port_leds(priv, port, qca8k_port_to_phy(port_num));
> > >
> > > As I checked, the function "qca8k_port_to_phy()" can return all 0xFFs
> > > for port_num == 0. Then, this value is implicitly casted to int (as the
> > > last parameter of "qca8k_parse_port_leds()"). Internally, in
> > > "qca8k_parse_port_leds()" this parameter can be used to do some
> > > computing - that looks dangerous.
> > > In summary, I think a special check for CPU port_num == 0 should be
> > > added.
> > > (I guess the LED configuration i only makes sense for non-CPU ports? It
> > > seems you want to configure up to 15 LEDs in total for 5 ports).
> > >
> >
> > IMHO for this, we can ignore handling this corner case. The hw doesn't
> > supports leds for port0 and port6 (the 2 CPU port) so the case won't
> > ever apply. But if asked I can add the case, not that it will cause any
> > problem in how the regs and shift are referenced in the code.
>
> OK, got it. So, as I understand, the previous call in this loop
> "fwnode_property_read_u32()" will never return port_num == 0 (or we fall
> into "continue")?
>
On a second check I got what you mean with this and I added a check to
skip LED init for port 0 and port 6. Thanks!
> >
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> > > > index 4e48e4dd8b0f..3c3c072fa9c2 100644
> > > > --- a/drivers/net/dsa/qca/qca8k.h
> > > > +++ b/drivers/net/dsa/qca/qca8k.h
> > > > @@ -11,6 +11,7 @@
> > > > #include <linux/delay.h>
> > > > #include <linux/regmap.h>
> > > > #include <linux/gpio.h>
> > > > +#include <linux/leds.h>
> > > > #include <linux/dsa/tag_qca.h>
> > > >
> > > > #define QCA8K_ETHERNET_MDIO_PRIORITY 7
> > > > @@ -85,6 +86,50 @@
> > > > #define QCA8K_MDIO_MASTER_DATA(x) FIELD_PREP(QCA8K_MDIO_MASTER_DATA_MASK, x)
> > > > #define QCA8K_MDIO_MASTER_MAX_PORTS 5
> > > > #define QCA8K_MDIO_MASTER_MAX_REG 32
> > > > +
> > > > +/* LED control register */
> > > > +#define QCA8K_LED_COUNT 15
> > > > +#define QCA8K_LED_PORT_COUNT 3
> > > > +#define QCA8K_LED_RULE_COUNT 6
> > > > +#define QCA8K_LED_RULE_MAX 11
> > > > +
> > > > +#define QCA8K_LED_PHY123_PATTERN_EN_SHIFT(_phy, _led) ((((_phy) - 1) * 6) + 8 + (2 * (_led)))
> > > > +#define QCA8K_LED_PHY123_PATTERN_EN_MASK GENMASK(1, 0)
> > > > +
> > > > +#define QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT 0
> > > > +#define QCA8K_LED_PHY4_CONTROL_RULE_SHIFT 16
> > > > +
> > > > +#define QCA8K_LED_CTRL_REG(_i) (0x050 + (_i) * 4)
> > > > +#define QCA8K_LED_CTRL0_REG 0x50
> > > > +#define QCA8K_LED_CTRL1_REG 0x54
> > > > +#define QCA8K_LED_CTRL2_REG 0x58
> > > > +#define QCA8K_LED_CTRL3_REG 0x5C
> > > > +#define QCA8K_LED_CTRL_SHIFT(_i) (((_i) % 2) * 16)
> > > > +#define QCA8K_LED_CTRL_MASK GENMASK(15, 0)
> > > > +#define QCA8K_LED_RULE_MASK GENMASK(13, 0)
> > > > +#define QCA8K_LED_BLINK_FREQ_MASK GENMASK(1, 0)
> > > > +#define QCA8K_LED_BLINK_FREQ_SHITF 0
> > > > +#define QCA8K_LED_BLINK_2HZ 0
> > > > +#define QCA8K_LED_BLINK_4HZ 1
> > > > +#define QCA8K_LED_BLINK_8HZ 2
> > > > +#define QCA8K_LED_BLINK_AUTO 3
> > > > +#define QCA8K_LED_LINKUP_OVER_MASK BIT(2)
> > > > +#define QCA8K_LED_TX_BLINK_MASK BIT(4)
> > > > +#define QCA8K_LED_RX_BLINK_MASK BIT(5)
> > > > +#define QCA8K_LED_COL_BLINK_MASK BIT(7)
> > > > +#define QCA8K_LED_LINK_10M_EN_MASK BIT(8)
> > > > +#define QCA8K_LED_LINK_100M_EN_MASK BIT(9)
> > > > +#define QCA8K_LED_LINK_1000M_EN_MASK BIT(10)
> > > > +#define QCA8K_LED_POWER_ON_LIGHT_MASK BIT(11)
> > > > +#define QCA8K_LED_HALF_DUPLEX_MASK BIT(12)
> > > > +#define QCA8K_LED_FULL_DUPLEX_MASK BIT(13)
> > > > +#define QCA8K_LED_PATTERN_EN_MASK GENMASK(15, 14)
> > > > +#define QCA8K_LED_PATTERN_EN_SHIFT 14
> > > > +#define QCA8K_LED_ALWAYS_OFF 0
> > > > +#define QCA8K_LED_ALWAYS_BLINK_4HZ 1
> > > > +#define QCA8K_LED_ALWAYS_ON 2
> > > > +#define QCA8K_LED_RULE_CONTROLLED 3
> > > > +
> > > > #define QCA8K_GOL_MAC_ADDR0 0x60
> > > > #define QCA8K_GOL_MAC_ADDR1 0x64
> > > > #define QCA8K_MAX_FRAME_SIZE 0x78
> > > > @@ -383,6 +428,19 @@ struct qca8k_pcs {
> > > > int port;
> > > > };
> > > >
> > > > +struct qca8k_led_pattern_en {
> > > > + u32 reg;
> > > > + u8 shift;
> > > > +};
> > > > +
> > > > +struct qca8k_led {
> > > > + u8 port_num;
> > > > + u8 led_num;
> > > > + u16 old_rule;
> > > > + struct qca8k_priv *priv;
> > > > + struct led_classdev cdev;
> > > > +};
> > > > +
> > > > struct qca8k_priv {
> > > > u8 switch_id;
> > > > u8 switch_revision;
> > > > @@ -407,6 +465,7 @@ struct qca8k_priv {
> > > > struct qca8k_pcs pcs_port_0;
> > > > struct qca8k_pcs pcs_port_6;
> > > > const struct qca8k_match_data *info;
> > > > + struct qca8k_led ports_led[QCA8K_LED_COUNT];
> > > > };
> > > >
> > > > struct qca8k_mib_desc {
> > > > diff --git a/drivers/net/dsa/qca/qca8k_leds.h b/drivers/net/dsa/qca/qca8k_leds.h
> > > > new file mode 100644
> > > > index 000000000000..ab367f05b173
> > > > --- /dev/null
> > > > +++ b/drivers/net/dsa/qca/qca8k_leds.h
> > > > @@ -0,0 +1,16 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +
> > > > +#ifndef __QCA8K_LEDS_H
> > > > +#define __QCA8K_LEDS_H
> > > > +
> > > > +/* Leds Support function */
> > > > +#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
> > > > +int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
> > > > +#else
> > > > +static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
> > > > +{
> > > > + return 0;
> > > > +}
> > > > +#endif
> > > > +
> > > > +#endif /* __QCA8K_LEDS_H */
> > > > --
> > > > 2.39.2
> > > >
> >
> > --
> > Ansuel
>
>
> Thanks,
> Michal
--
Ansuel
On Fri, Mar 17, 2023 at 12:54:09PM +0100, Michal Kubiak wrote:
> On Fri, Mar 17, 2023 at 03:31:14AM +0100, Christian Marangi wrote:
> > Add LEDs blink_set() support to qca8k Switch Family.
> > These LEDs support hw accellerated blinking at a fixed rate
> > of 4Hz.
> >
> > Reject any other value since not supported by the LEDs switch.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
> > Reviewed-by: Andrew Lunn <[email protected]>
> > ---
> > drivers/net/dsa/qca/qca8k-leds.c | 38 ++++++++++++++++++++++++++++++++
> > 1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
> > index adbe7f6e2994..c229575c7e8c 100644
> > --- a/drivers/net/dsa/qca/qca8k-leds.c
> > +++ b/drivers/net/dsa/qca/qca8k-leds.c
> > @@ -92,6 +92,43 @@ qca8k_led_brightness_get(struct qca8k_led *led)
> > return val == QCA8K_LED_ALWAYS_ON;
> > }
> >
> > +static int
> > +qca8k_cled_blink_set(struct led_classdev *ldev,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off)
> > +{
> > + struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > + u32 mask, val = QCA8K_LED_ALWAYS_BLINK_4HZ;
> > + struct qca8k_led_pattern_en reg_info;
> > + struct qca8k_priv *priv = led->priv;
> > +
> > + if (*delay_on == 0 && *delay_off == 0) {
> > + *delay_on = 125;
> > + *delay_off = 125;
> > + }
> > +
> > + if (*delay_on != 125 || *delay_off != 125) {
> > + /* The hardware only supports blinking at 4Hz. Fall back
> > + * to software implementation in other cases.
> > + */
> > + return -EINVAL;
> > + }
> > +
> > + qca8k_get_enable_led_reg(led->port_num, led->led_num, ®_info);
> > +
> > + if (led->port_num == 0 || led->port_num == 4) {
> > + mask = QCA8K_LED_PATTERN_EN_MASK;
> > + val <<= QCA8K_LED_PATTERN_EN_SHIFT;
> > + } else {
> > + mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
> > + }
>
> Could you add a comment as to why the HW requires different approaches for
> inner and outer ports?
>
Since they are really the same thing, I added a big comment that explain
how the thing mask shift and reg works and why in the brightness_set
function.
> > +
> > + regmap_update_bits(priv->regmap, reg_info.reg, mask << reg_info.shift,
> > + val << reg_info.shift);
> > +
> > + return 0;
> > +}
> > +
> > static int
> > qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
> > {
> > @@ -149,6 +186,7 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p
> >
> > port_led->cdev.max_brightness = 1;
> > port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
> > + port_led->cdev.blink_set = qca8k_cled_blink_set;
> > init_data.default_label = ":port";
> > init_data.devicename = "qca8k";
> > init_data.fwnode = led;
> > --
> > 2.39.2
> >
>
>
> Thanks,
> Michal
--
Ansuel