[net-next,PATCH 1/2] net: dsa: microchip: Add emulated MIIM access to switch LED config registers

[net-next,PATCH 1/2] net: dsa: microchip: Add emulated MIIM access to switch LED config registers

[Marek Vasut]

The KSZ87xx switch DSA driver binds a simplified KSZ8795 switch PHY driver to
each port. The KSZ8795 switch PHY driver is part of drivers/net/phy/micrel.c
and uses generic PHY register accessors to access MIIM registers 0x00..0x05,
0x1d and 0x1f . The MII access is implemented by the KSZ87xx switch DSA driver
and internally done over whichever interface the KSZ87xx switch is connected
to the SoC.

In order to configure LEDs from the KSZ8795 switch PHY driver, it is necessary
to expose the LED control registers to the PHY driver, however, the LED control
registers are not part of the MIIM block, they are in Switch Config Registers
block.

This preparatory patch exposes the LED control bits in those Switch Config
Registers by mapping them at high addresses in the MIIM space, so the PHY
driver can access those registers and surely not collide with the existing
MIIM block registers. The two registers which are exposed are the global
Register 11 (0x0B): Global Control 9 as MIIM block register 0x0b00 and
port specific Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3 Control 10
as MIIM block register 0x0d00 . The access to those registers is further
restricted only to the LED configuration bits to prevent the PHY driver
or userspace tools like 'phytool' from tampering with any other switch
configuration through this interface.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Tristram Ha <tristram.ha@microchip.com>
Cc: UNGLinuxDriver@microchip.com
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Woojung Huh <woojung.huh@microchip.com>
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 drivers/net/dsa/microchip/ksz8.c | 47 ++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
index da7110d675583..9698bf53378af 100644
--- a/drivers/net/dsa/microchip/ksz8.c
+++ b/drivers/net/dsa/microchip/ksz8.c
@@ -1044,6 +1044,22 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 			return ret;
 
 		break;
+	/* Emulated access to Register 11 (0x0B): Global Control 9 */
+	case (REG_SW_CTRL_9 << 8):
+		ret = ksz_read8(dev, REG_SW_CTRL_9, &val1);
+		if (ret)
+			return ret;
+
+		data = val1 & 0x30;	/* LED Mode */
+		break;
+	/* Emulated access to Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3 Control 10 */
+	case (REG_PORT_CTRL_10 << 8):
+		ret = ksz_pread8(dev, p, REG_PORT_CTRL_10, &val1);
+		if (ret)
+			return ret;
+
+		data = val1 & BIT(7);	/* LED Off */
+		break;
 	default:
 		processed = false;
 		break;
@@ -1256,6 +1272,37 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 		if (ret)
 			return ret;
 		break;
+
+	/* Emulated access to Register 11 (0x0B): Global Control 9 */
+	case (REG_SW_CTRL_9 << 8):
+		ret = ksz_read8(dev, REG_SW_CTRL_9, &data);
+		if (ret)
+			return ret;
+
+		/* Only ever allow LED Mode update */
+		data &= ~0x30;
+		data |= val & 0x30;
+
+		ret = ksz_write8(dev, REG_SW_CTRL_9, data);
+		if (ret)
+			return ret;
+		break;
+
+	/* Emulated access to Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3 Control 10 */
+	case (REG_PORT_CTRL_10 << 8):
+		ret = ksz_pread8(dev, p, REG_PORT_CTRL_10, &data);
+		if (ret)
+			return ret;
+
+		/* Only ever allow LED Off update */
+		data &= ~BIT(7);
+		data |= val & BIT(7);
+
+		ret = ksz_pwrite8(dev, p, REG_PORT_CTRL_10, data);
+		if (ret)
+			return ret;
+		break;
+
 	default:
 		break;
 	}
-- 
2.45.2

[net-next,PATCH 2/2] net: phy: micrel: Add KSZ87XX Switch LED control

[Marek Vasut]

The KSZ87xx switch contains LED control registers. There is one shared
global control register bitfield which affects behavior of all LEDs on
all ports, the Register 11 (0x0B): Global Control 9 bitfield [5:4].
There is also one per-port Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3
Control 10 bit 7 which controls enablement of both LEDs on each port
separately.

Expose LED brightness control and HW offload support for both of the two
programmable LEDs on this KSZ87XX Switch. Note that on KSZ87xx there are
three or more instances of simple KSZ87XX Switch PHY, one for each port,
however, the registers which control the LED behavior are mostly shared.

Introduce LED brightness control using Register 29/45/61 (0x1D/0x2D/0x3D):
Port 1/2/3 Control 10 bit 7. This bit selects between LEDs disabled and
LEDs set to Function mode. In case LED brightness is set to 0, both LEDs
are turned off, otherwise both LEDs are configured to Function mode which
follows the global Register 11 (0x0B): Global Control 9 bitfield [5:4]
setting.

Note that while two LEDs are registered per port, and each expose a matching
sysfs directory which contains a brightness attribute, a write into either
brightness attribute does reconfigure the same bit 7 in Register 29/45/61
(0x1D/0x2D/0x3D): Port 1/2/3 Control 10 for that particular port . The two
brightness attributes can also be out of sync which is not great.

Introduce LED mode configuration using Register 11 (0x0B): Global Control
9 bitfield [5:4]. This bitfield can be set to 1 of 4 non-orthogonal mode
settings which affects both LEDs on the port. Use a look up table to find
out whether setting one LED on the port is compatible with current setting
of the other LED and if not, reject the configuration until both LEDs are
configured to one of the four valid modes.

Note that while there are two LEDs per port, and there are multiple ports,
each with matching sysfs directory which contains netdev trigger attributes,
a write into either attribute does reconfigure the same shared Register 11
(0x0B): Global Control 9 bitfield [5:4] and the sysfs attributes can be out
of sync which is not great.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Tristram Ha <tristram.ha@microchip.com>
Cc: UNGLinuxDriver@microchip.com
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Woojung Huh <woojung.huh@microchip.com>
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 drivers/net/phy/micrel.c | 112 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index eeb33eb181ac9..08eda25852048 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -434,6 +434,7 @@ struct kszphy_priv {
 	const struct kszphy_type *type;
 	struct clk *clk;
 	int led_mode;
+	unsigned long led_rules[2];
 	u16 vct_ctrl1000;
 	bool rmii_ref_clk_sel;
 	bool rmii_ref_clk_sel_val;
@@ -891,6 +892,112 @@ static int ksz8795_match_phy_device(struct phy_device *phydev)
 	return ksz8051_ksz8795_match_phy_device(phydev, false);
 }
 
+#define KSZ8795_LED_COUNT	2
+
+static const unsigned long ksz8795_led_rules_map[4][2] = {
+	{
+		/* Control Bits = 2'b00 => LEDx_0=Link/ACT LEDx_1=Speed */
+		BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_RX) |
+		BIT(TRIGGER_NETDEV_TX),
+		BIT(TRIGGER_NETDEV_LINK_100)
+	}, {
+		/* Control Bits = 2'b01 => LEDx_0=Link     LEDx_1=ACT */
+		BIT(TRIGGER_NETDEV_LINK),
+		BIT(TRIGGER_NETDEV_RX) | BIT(TRIGGER_NETDEV_TX)
+	}, {
+		/* Control Bits = 2'b10 => LEDx_0=Link/ACT LEDx_1=Duplex */
+		BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_RX) |
+		BIT(TRIGGER_NETDEV_TX),
+		BIT(TRIGGER_NETDEV_FULL_DUPLEX)
+	}, {
+		/* Control Bits = 2'b11 => LEDx_0=Link     LEDx_1=Duplex */
+		BIT(TRIGGER_NETDEV_LINK),
+		BIT(TRIGGER_NETDEV_FULL_DUPLEX)
+	}
+};
+
+static int ksz8795_led_brightness_set(struct phy_device *phydev, u8 index,
+				      enum led_brightness value)
+{
+	/* Turn all LEDs on this port on or off */
+	/* Emulated rmw of Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3 Control 10 */
+	return phy_modify(phydev, 0x0d00, BIT(7), (value == LED_OFF) ? BIT(7) : 0);
+}
+
+static int ksz8795_led_hw_is_supported(struct phy_device *phydev, u8 index,
+				       unsigned long rules)
+{
+	const unsigned long mask[2] = {
+		BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_RX) |
+		BIT(TRIGGER_NETDEV_TX),
+		BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_RX) |
+		BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_FULL_DUPLEX)
+	};
+
+	if (index >= KSZ8795_LED_COUNT)
+		return -EINVAL;
+
+	/* Filter out any other unsupported triggers. */
+	if (rules & ~mask[index])
+		return -EOPNOTSUPP;
+
+	/* RX and TX are not differentiated, either both are set or not set. */
+	if (!(rules & BIT(TRIGGER_NETDEV_RX)) ^ !(rules & BIT(TRIGGER_NETDEV_TX)))
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+static int ksz8795_led_hw_control_get(struct phy_device *phydev, u8 index,
+				      unsigned long *rules)
+{
+	int val;
+
+	if (index >= KSZ8795_LED_COUNT)
+		return -EINVAL;
+
+	/* Emulated read of Register 11 (0x0B): Global Control 9 */
+	val = phy_read(phydev, 0x0b00);
+	if (val < 0)
+		return val;
+
+	/* Extract bits [5:4] and look up matching LED configuration */
+	*rules = ksz8795_led_rules_map[(val >> 4) & 0x3][index];
+
+	return 0;
+}
+
+static int ksz8795_led_hw_control_set(struct phy_device *phydev, u8 index,
+				      unsigned long rules)
+{
+	struct kszphy_priv *priv = phydev->priv;
+	unsigned long other_rules;
+	int i;
+
+	if (index >= KSZ8795_LED_COUNT)
+		return -EINVAL;
+
+	/*
+	 * Cache the rules for this LED for future use when setting up the
+	 * other LED and looking up compatible configuration of the global
+	 * control 9 register bitfield [5:4].
+	 */
+	priv->led_rules[index] = rules;
+
+	/* Use cached configuration of the other LED. */
+	other_rules = priv->led_rules[!index];
+
+	/* Update this LED configuration if compatible with the other LED */
+	for (i = 0; i < 4; i++) {
+		if (ksz8795_led_rules_map[i][index] == rules &&
+		    ksz8795_led_rules_map[i][!index] == other_rules) {
+			return phy_modify(phydev, 0x0b00, 0x30, i << 4);
+		}
+	}
+
+	return -EINVAL;
+}
+
 static int ksz9021_load_values_from_of(struct phy_device *phydev,
 				       const struct device_node *of_node,
 				       u16 reg,
@@ -5666,10 +5773,15 @@ static struct phy_driver ksphy_driver[] = {
 }, {
 	.name		= "Micrel KSZ87XX Switch",
 	/* PHY_BASIC_FEATURES */
+	.probe		= kszphy_probe,
 	.config_init	= kszphy_config_init,
 	.match_phy_device = ksz8795_match_phy_device,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
+	.led_brightness_set = ksz8795_led_brightness_set,
+	.led_hw_is_supported = ksz8795_led_hw_is_supported,
+	.led_hw_control_get = ksz8795_led_hw_control_get,
+	.led_hw_control_set = ksz8795_led_hw_control_set,
 }, {
 	.phy_id		= PHY_ID_KSZ9477,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
-- 
2.45.2

Re: [net-next,PATCH 1/2] net: dsa: microchip: Add emulated MIIM access to switch LED config registers

[Paolo Abeni]

On 1/13/25 1:15 AM, Marek Vasut wrote:
> The KSZ87xx switch DSA driver binds a simplified KSZ8795 switch PHY driver to
> each port. The KSZ8795 switch PHY driver is part of drivers/net/phy/micrel.c
> and uses generic PHY register accessors to access MIIM registers 0x00..0x05,
> 0x1d and 0x1f . The MII access is implemented by the KSZ87xx switch DSA driver
> and internally done over whichever interface the KSZ87xx switch is connected
> to the SoC.
> 
> In order to configure LEDs from the KSZ8795 switch PHY driver, it is necessary
> to expose the LED control registers to the PHY driver, however, the LED control
> registers are not part of the MIIM block, they are in Switch Config Registers
> block.
> 
> This preparatory patch exposes the LED control bits in those Switch Config
> Registers by mapping them at high addresses in the MIIM space, so the PHY
> driver can access those registers and surely not collide with the existing
> MIIM block registers. The two registers which are exposed are the global
> Register 11 (0x0B): Global Control 9 as MIIM block register 0x0b00 and
> port specific Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3 Control 10
> as MIIM block register 0x0d00 . The access to those registers is further
> restricted only to the LED configuration bits to prevent the PHY driver
> or userspace tools like 'phytool' from tampering with any other switch
> configuration through this interface.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Tristram Ha <tristram.ha@microchip.com>
> Cc: UNGLinuxDriver@microchip.com
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Woojung Huh <woojung.huh@microchip.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> ---
>  drivers/net/dsa/microchip/ksz8.c | 47 ++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index da7110d675583..9698bf53378af 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -1044,6 +1044,22 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
>  			return ret;
>  
>  		break;
> +	/* Emulated access to Register 11 (0x0B): Global Control 9 */
> +	case (REG_SW_CTRL_9 << 8):

I think it's better to add a macro for the new registers

> +		ret = ksz_read8(dev, REG_SW_CTRL_9, &val1);
> +		if (ret)
> +			return ret;
> +
> +		data = val1 & 0x30;	/* LED Mode */

In a similar way, it's better to add meaningful macros for the relevant
register fields. You should consider using the *FIELD*() helpers, too.

/P

Re: [net-next,PATCH 2/2] net: phy: micrel: Add KSZ87XX Switch LED control

[Paolo Abeni]

On 1/13/25 1:15 AM, Marek Vasut wrote:
> The KSZ87xx switch contains LED control registers. There is one shared
> global control register bitfield which affects behavior of all LEDs on
> all ports, the Register 11 (0x0B): Global Control 9 bitfield [5:4].
> There is also one per-port Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3
> Control 10 bit 7 which controls enablement of both LEDs on each port
> separately.
> 
> Expose LED brightness control and HW offload support for both of the two
> programmable LEDs on this KSZ87XX Switch. Note that on KSZ87xx there are
> three or more instances of simple KSZ87XX Switch PHY, one for each port,
> however, the registers which control the LED behavior are mostly shared.
> 
> Introduce LED brightness control using Register 29/45/61 (0x1D/0x2D/0x3D):
> Port 1/2/3 Control 10 bit 7. This bit selects between LEDs disabled and
> LEDs set to Function mode. In case LED brightness is set to 0, both LEDs
> are turned off, otherwise both LEDs are configured to Function mode which
> follows the global Register 11 (0x0B): Global Control 9 bitfield [5:4]
> setting.

@Andrew, @Russel: can the above problem be address with the current phy
API? or does phy device need to expose a new brightness_get op?

[...]
> @@ -891,6 +892,112 @@ static int ksz8795_match_phy_device(struct phy_device *phydev)
>  	return ksz8051_ksz8795_match_phy_device(phydev, false);
>  }
>  
> +#define KSZ8795_LED_COUNT	2
> +
> +static const unsigned long ksz8795_led_rules_map[4][2] = {
> +	{
> +		/* Control Bits = 2'b00 => LEDx_0=Link/ACT LEDx_1=Speed */
> +		BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_RX) |
> +		BIT(TRIGGER_NETDEV_TX),
> +		BIT(TRIGGER_NETDEV_LINK_100)
> +	}, {
> +		/* Control Bits = 2'b01 => LEDx_0=Link     LEDx_1=ACT */
> +		BIT(TRIGGER_NETDEV_LINK),
> +		BIT(TRIGGER_NETDEV_RX) | BIT(TRIGGER_NETDEV_TX)
> +	}, {
> +		/* Control Bits = 2'b10 => LEDx_0=Link/ACT LEDx_1=Duplex */
> +		BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_RX) |
> +		BIT(TRIGGER_NETDEV_TX),
> +		BIT(TRIGGER_NETDEV_FULL_DUPLEX)
> +	}, {
> +		/* Control Bits = 2'b11 => LEDx_0=Link     LEDx_1=Duplex */
> +		BIT(TRIGGER_NETDEV_LINK),
> +		BIT(TRIGGER_NETDEV_FULL_DUPLEX)
> +	}
> +};
> +
> +static int ksz8795_led_brightness_set(struct phy_device *phydev, u8 index,
> +				      enum led_brightness value)
> +{
> +	/* Turn all LEDs on this port on or off */
> +	/* Emulated rmw of Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3 Control 10 */
> +	return phy_modify(phydev, 0x0d00, BIT(7), (value == LED_OFF) ? BIT(7) : 0);

Please defines macros for all the above 'magic numbers'

> +}
> +
> +static int ksz8795_led_hw_is_supported(struct phy_device *phydev, u8 index,
> +				       unsigned long rules)
> +{
> +	const unsigned long mask[2] = {
> +		BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_RX) |
> +		BIT(TRIGGER_NETDEV_TX),
> +		BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_RX) |
> +		BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_FULL_DUPLEX)
> +	};
> +
> +	if (index >= KSZ8795_LED_COUNT)
> +		return -EINVAL;
> +
> +	/* Filter out any other unsupported triggers. */
> +	if (rules & ~mask[index])
> +		return -EOPNOTSUPP;
> +
> +	/* RX and TX are not differentiated, either both are set or not set. */
> +	if (!(rules & BIT(TRIGGER_NETDEV_RX)) ^ !(rules & BIT(TRIGGER_NETDEV_TX)))
> +		return -EOPNOTSUPP;
> +
> +	return 0;
> +}
> +
> +static int ksz8795_led_hw_control_get(struct phy_device *phydev, u8 index,
> +				      unsigned long *rules)
> +{
> +	int val;
> +
> +	if (index >= KSZ8795_LED_COUNT)
> +		return -EINVAL;
> +
> +	/* Emulated read of Register 11 (0x0B): Global Control 9 */
> +	val = phy_read(phydev, 0x0b00);
> +	if (val < 0)
> +		return val;
> +
> +	/* Extract bits [5:4] and look up matching LED configuration */
> +	*rules = ksz8795_led_rules_map[(val >> 4) & 0x3][index];

This calls for FIELD_GET() usage.

[...]
> @@ -5666,10 +5773,15 @@ static struct phy_driver ksphy_driver[] = {
>  }, {
>  	.name		= "Micrel KSZ87XX Switch",
>  	/* PHY_BASIC_FEATURES */
> +	.probe		= kszphy_probe,
>  	.config_init	= kszphy_config_init,
>  	.match_phy_device = ksz8795_match_phy_device,
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
> +	.led_brightness_set = ksz8795_led_brightness_set,
> +	.led_hw_is_supported = ksz8795_led_hw_is_supported,
> +	.led_hw_control_get = ksz8795_led_hw_control_get,
> +	.led_hw_control_set = ksz8795_led_hw_control_set,

The preferred style is to use an additional tab to align all the '=' in
this new block.

/P

Re: [net-next,PATCH 1/2] net: dsa: microchip: Add emulated MIIM access to switch LED config registers

[Andrew Lunn]

> This preparatory patch exposes the LED control bits in those Switch Config
> Registers by mapping them at high addresses in the MIIM space, so the PHY
> driver can access those registers and surely not collide with the existing
> MIIM block registers. The two registers which are exposed are the global
> Register 11 (0x0B): Global Control 9 as MIIM block register 0x0b00 and
> port specific Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3 Control 10
> as MIIM block register 0x0d00 . The access to those registers is further
> restricted only to the LED configuration bits to prevent the PHY driver
> or userspace tools like 'phytool' from tampering with any other switch
> configuration through this interface.

I do wounder about using register numbers outside of the 0-31
range. We never have enforced it, but these clearly break 802.3 C22.

I wounder if it would be better to emulate pages as well. The PHY
driver already has lanphy_write_page_reg() and
lanphy_read_page_reg(). Could you emulate what those need?  It adds a
lot of complexity, but it should be future proof.

What might be simpler is to expose an emulated C45 register range, and
put the registers in the vendor section.


	Andrew

Re: [net-next,PATCH 2/2] net: phy: micrel: Add KSZ87XX Switch LED control

[Andrew Lunn]

On Thu, Jan 16, 2025 at 10:58:38AM +0100, Paolo Abeni wrote:
> On 1/13/25 1:15 AM, Marek Vasut wrote:
> > The KSZ87xx switch contains LED control registers. There is one shared
> > global control register bitfield which affects behavior of all LEDs on
> > all ports, the Register 11 (0x0B): Global Control 9 bitfield [5:4].
> > There is also one per-port Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3
> > Control 10 bit 7 which controls enablement of both LEDs on each port
> > separately.
> > 
> > Expose LED brightness control and HW offload support for both of the two
> > programmable LEDs on this KSZ87XX Switch. Note that on KSZ87xx there are
> > three or more instances of simple KSZ87XX Switch PHY, one for each port,
> > however, the registers which control the LED behavior are mostly shared.
> > 
> > Introduce LED brightness control using Register 29/45/61 (0x1D/0x2D/0x3D):
> > Port 1/2/3 Control 10 bit 7. This bit selects between LEDs disabled and
> > LEDs set to Function mode. In case LED brightness is set to 0, both LEDs
> > are turned off, otherwise both LEDs are configured to Function mode which
> > follows the global Register 11 (0x0B): Global Control 9 bitfield [5:4]
> > setting.
> 
> @Andrew, @Russel: can the above problem be address with the current phy
> API? or does phy device need to expose a new brightness_get op?

Coupled LEDs cause issues. Because vendors do all sorts of weird
things with LEDs, i don't want to fully support everything every
vendor has, or the code is just going to be unmaintanable. So we have
the core support a subset, and some features of the hardware go
unused.

In this case, i would say software control of the LEDs is
impossible. You cannot individually turn an LED on/off. The core
supports this, Heiner added support for a PHY with similar issues. I
think it is as simple as not implementing led_brightness_set().


    Andrew

---
pw-bot: cr

Re: [net-next,PATCH 1/2] net: dsa: microchip: Add emulated MIIM access to switch LED config registers

[Marek Vasut]

On 1/16/25 2:06 PM, Andrew Lunn wrote:
>> This preparatory patch exposes the LED control bits in those Switch Config
>> Registers by mapping them at high addresses in the MIIM space, so the PHY
>> driver can access those registers and surely not collide with the existing
>> MIIM block registers. The two registers which are exposed are the global
>> Register 11 (0x0B): Global Control 9 as MIIM block register 0x0b00 and
>> port specific Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3 Control 10
>> as MIIM block register 0x0d00 . The access to those registers is further
>> restricted only to the LED configuration bits to prevent the PHY driver
>> or userspace tools like 'phytool' from tampering with any other switch
>> configuration through this interface.
> 
> I do wounder about using register numbers outside of the 0-31
> range. We never have enforced it, but these clearly break 802.3 C22.

Yes, I agree this is completely non-standard.

The registers I need to access from the PHY driver are neither C22 nor 
C45 registers , they are switch registers accessible directly via SPI 
only. I only needed some way to accesses them from the PHY driver to 
flip the LED bits.

> I wounder if it would be better to emulate pages as well. The PHY
> driver already has lanphy_write_page_reg() and
> lanphy_read_page_reg(). Could you emulate what those need?  It adds a
> lot of complexity, but it should be future proof.
> 
> What might be simpler is to expose an emulated C45 register range, and
> put the registers in the vendor section.
What would be the benefit of doing that, compared to the current 
approach? I think in either case, for example phytool could tamper with 
those registers anyway, right ?

Please note that the KSZ9xxx series to my knowledge do not have this PHY 
interface anymore, so that future proofing part is not really applicable 
I think.

Re: [net-next,PATCH 2/2] net: phy: micrel: Add KSZ87XX Switch LED control

[Marek Vasut]

On 1/16/25 2:23 PM, Andrew Lunn wrote:
> On Thu, Jan 16, 2025 at 10:58:38AM +0100, Paolo Abeni wrote:
>> On 1/13/25 1:15 AM, Marek Vasut wrote:
>>> The KSZ87xx switch contains LED control registers. There is one shared
>>> global control register bitfield which affects behavior of all LEDs on
>>> all ports, the Register 11 (0x0B): Global Control 9 bitfield [5:4].
>>> There is also one per-port Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3
>>> Control 10 bit 7 which controls enablement of both LEDs on each port
>>> separately.
>>>
>>> Expose LED brightness control and HW offload support for both of the two
>>> programmable LEDs on this KSZ87XX Switch. Note that on KSZ87xx there are
>>> three or more instances of simple KSZ87XX Switch PHY, one for each port,
>>> however, the registers which control the LED behavior are mostly shared.
>>>
>>> Introduce LED brightness control using Register 29/45/61 (0x1D/0x2D/0x3D):
>>> Port 1/2/3 Control 10 bit 7. This bit selects between LEDs disabled and
>>> LEDs set to Function mode. In case LED brightness is set to 0, both LEDs
>>> are turned off, otherwise both LEDs are configured to Function mode which
>>> follows the global Register 11 (0x0B): Global Control 9 bitfield [5:4]
>>> setting.
>>
>> @Andrew, @Russel: can the above problem be address with the current phy
>> API? or does phy device need to expose a new brightness_get op?

Sorry for the delayed reply.

> Coupled LEDs cause issues. Because vendors do all sorts of weird
> things with LEDs, i don't want to fully support everything every
> vendor has, or the code is just going to be unmaintanable. So we have
> the core support a subset, and some features of the hardware go
> unused.
> 
> In this case, i would say software control of the LEDs is
> impossible. You cannot individually turn an LED on/off. The core
> supports this, Heiner added support for a PHY with similar issues. I
> think it is as simple as not implementing led_brightness_set().
What about the LED functionality configuration ?

I think the main issue here is that multiple LEDs exposed in sysfs 
control the same bit(s) in hardware, and the sysfs attributes behind 
those LEDs can get out of sync.

One alternative that crossed my mind would be to register single LED in 
the KSZ87xx switch driver to control the LED functionality, and per-port 
LED to control the per-port LED enablement (=brightness). It does not 
model the hardware accurately, but it does avoid the complexity.

Otherwise, what is left would be custom sysfs attributes for this switch?