| Message ID | 20240201151747.7524-10-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: Introduce PHY Package concept | expand |
> + > + phydev->drv->led_brightness_set = NULL; > + phydev->drv->led_blink_set = NULL; > + phydev->drv->led_hw_is_supported = NULL; > + phydev->drv->led_hw_control_set = NULL; > + phydev->drv->led_hw_control_get = NULL; I don't see how that works. You have multiple PHYs using this driver. Some might have LEDs, some might have GPOs. But if you modify the driver structure like this, you prevent all PHYs from having LEDs, and maybe cause a Opps if a PHY device has already registered its LEDs? Andrew
On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > + > > + phydev->drv->led_brightness_set = NULL; > > + phydev->drv->led_blink_set = NULL; > > + phydev->drv->led_hw_is_supported = NULL; > > + phydev->drv->led_hw_control_set = NULL; > > + phydev->drv->led_hw_control_get = NULL; > > I don't see how that works. You have multiple PHYs using this > driver. Some might have LEDs, some might have GPOs. But if you modify > the driver structure like this, you prevent all PHYs from having LEDs, > and maybe cause a Opps if a PHY device has already registered its > LEDs? > God you are right! Off-topic but given the effects this may cause, why the thing is not const? I assume it wouldn't make sense to add OPS based on the detected feature since it would have side effect on other PHYs that use the same driver.
On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote: > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > > + > > > + phydev->drv->led_brightness_set = NULL; > > > + phydev->drv->led_blink_set = NULL; > > > + phydev->drv->led_hw_is_supported = NULL; > > > + phydev->drv->led_hw_control_set = NULL; > > > + phydev->drv->led_hw_control_get = NULL; > > > > I don't see how that works. You have multiple PHYs using this > > driver. Some might have LEDs, some might have GPOs. But if you modify > > the driver structure like this, you prevent all PHYs from having LEDs, > > and maybe cause a Opps if a PHY device has already registered its > > LEDs? > > > > God you are right! Off-topic but given the effects this may cause, why > the thing is not const? I assume it wouldn't make sense to add OPS based > on the detected feature since it would have side effect on other PHYs > that use the same driver. Maybe phydev->drv should be const to avoid this kind of thing. It doesn't look like it would be hard to do, and importantly doesn't require casting away the const-ness anywhere. PHY drivers themselves can't be const because the driver model needs to be able to modify the embedded device_driver struct (e.g. see bus_add_driver().) drivers/net/phy/phy.c | 3 +-- drivers/net/phy/phy_device.c | 4 ++-- drivers/net/phy/xilinx_gmii2rgmii.c | 2 +- include/linux/phy.h | 2 +- 4 files changed, 5 insertions(+), 6 deletions(-) Just build-testing it.
On Fri, Feb 02, 2024 at 05:04:23PM +0000, Russell King (Oracle) wrote: > On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote: > > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > > > + > > > > + phydev->drv->led_brightness_set = NULL; > > > > + phydev->drv->led_blink_set = NULL; > > > > + phydev->drv->led_hw_is_supported = NULL; > > > > + phydev->drv->led_hw_control_set = NULL; > > > > + phydev->drv->led_hw_control_get = NULL; > > > > > > I don't see how that works. You have multiple PHYs using this > > > driver. Some might have LEDs, some might have GPOs. But if you modify > > > the driver structure like this, you prevent all PHYs from having LEDs, > > > and maybe cause a Opps if a PHY device has already registered its > > > LEDs? > > > > > > > God you are right! Off-topic but given the effects this may cause, why > > the thing is not const? I assume it wouldn't make sense to add OPS based > > on the detected feature since it would have side effect on other PHYs > > that use the same driver. > > Maybe phydev->drv should be const to avoid this kind of thing. It > doesn't look like it would be hard to do, and importantly doesn't > require casting away the const-ness anywhere. PHY drivers themselves > can't be const because the driver model needs to be able to modify > the embedded device_driver struct (e.g. see bus_add_driver().) > > drivers/net/phy/phy.c | 3 +-- > drivers/net/phy/phy_device.c | 4 ++-- > drivers/net/phy/xilinx_gmii2rgmii.c | 2 +- > include/linux/phy.h | 2 +- > 4 files changed, 5 insertions(+), 6 deletions(-) > > Just build-testing it. > Seems sensible to me. Also for everyone that does that (downstream or driver that needs to be handled) it would result in a warning for modifying const stuff. Maybe I'm wrong but I can only see benefits in doing this change.
On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote: > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > > + > > > + phydev->drv->led_brightness_set = NULL; > > > + phydev->drv->led_blink_set = NULL; > > > + phydev->drv->led_hw_is_supported = NULL; > > > + phydev->drv->led_hw_control_set = NULL; > > > + phydev->drv->led_hw_control_get = NULL; > > > > I don't see how that works. You have multiple PHYs using this > > driver. Some might have LEDs, some might have GPOs. But if you modify > > the driver structure like this, you prevent all PHYs from having LEDs, > > and maybe cause a Opps if a PHY device has already registered its > > LEDs? > > > > God you are right! Off-topic but given the effects this may cause, why > the thing is not const? I would like it to be, but its not easy. There are fields in the driver structure that phylib needs to modify. e.g. mdiodrv.driver gets passed to the driver core when registering the driver, and it modifies it. mdiodrv.flags is also manipulated. So we cannot make the whole structure const. Andrew
On Fri, Feb 02, 2024 at 06:08:33PM +0100, Andrew Lunn wrote: > On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote: > > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > > > + > > > > + phydev->drv->led_brightness_set = NULL; > > > > + phydev->drv->led_blink_set = NULL; > > > > + phydev->drv->led_hw_is_supported = NULL; > > > > + phydev->drv->led_hw_control_set = NULL; > > > > + phydev->drv->led_hw_control_get = NULL; > > > > > > I don't see how that works. You have multiple PHYs using this > > > driver. Some might have LEDs, some might have GPOs. But if you modify > > > the driver structure like this, you prevent all PHYs from having LEDs, > > > and maybe cause a Opps if a PHY device has already registered its > > > LEDs? > > > > > > > God you are right! Off-topic but given the effects this may cause, why > > the thing is not const? > > I would like it to be, but its not easy. There are fields in the > driver structure that phylib needs to modify. e.g. mdiodrv.driver gets > passed to the driver core when registering the driver, and it modifies > it. mdiodrv.flags is also manipulated. So we cannot make the whole > structure const. > Maybe the ops part can be detached and just that made const? (and introduce something like struct phy_driver_ops) It would require a big conversion but assuming nobody adds OPs in probe function everything should be static and easy to convert.
On Fri, Feb 02, 2024 at 06:08:33PM +0100, Andrew Lunn wrote: > On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote: > > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > > > + > > > > + phydev->drv->led_brightness_set = NULL; > > > > + phydev->drv->led_blink_set = NULL; > > > > + phydev->drv->led_hw_is_supported = NULL; > > > > + phydev->drv->led_hw_control_set = NULL; > > > > + phydev->drv->led_hw_control_get = NULL; > > > > > > I don't see how that works. You have multiple PHYs using this > > > driver. Some might have LEDs, some might have GPOs. But if you modify > > > the driver structure like this, you prevent all PHYs from having LEDs, > > > and maybe cause a Opps if a PHY device has already registered its > > > LEDs? > > > > > > > God you are right! Off-topic but given the effects this may cause, why > > the thing is not const? > > I would like it to be, but its not easy. There are fields in the > driver structure that phylib needs to modify. e.g. mdiodrv.driver gets > passed to the driver core when registering the driver, and it modifies > it. mdiodrv.flags is also manipulated. So we cannot make the whole > structure const. We can make phy_device's drv pointer const though, which would have the effect of catching code such as the above. That doesn't impact the driver model nor the mdio layer.
diff --git a/drivers/net/phy/qcom/qca807x.c b/drivers/net/phy/qcom/qca807x.c index 5ff6ba9fa1a0..0d7fd1a9404e 100644 --- a/drivers/net/phy/qcom/qca807x.c +++ b/drivers/net/phy/qcom/qca807x.c @@ -47,8 +47,18 @@ #define QCA807X_MMD7_LED_CTRL(x) (0x8074 + ((x) * 2)) #define QCA807X_MMD7_LED_FORCE_CTRL(x) (0x8075 + ((x) * 2)) -#define QCA807X_GPIO_FORCE_EN BIT(15) -#define QCA807X_GPIO_FORCE_MODE_MASK GENMASK(14, 13) +/* LED hw control pattern for fiber port */ +#define QCA807X_LED_FIBER_PATTERN_MASK GENMASK(11, 1) +#define QCA807X_LED_FIBER_TXACT_BLK_EN BIT(10) +#define QCA807X_LED_FIBER_RXACT_BLK_EN BIT(9) +#define QCA807X_LED_FIBER_FDX_ON_EN BIT(6) +#define QCA807X_LED_FIBER_HDX_ON_EN BIT(5) +#define QCA807X_LED_FIBER_1000BX_ON_EN BIT(2) +#define QCA807X_LED_FIBER_100FX_ON_EN BIT(1) + +/* Some device repurpose the LED as GPIO out */ +#define QCA807X_GPIO_FORCE_EN QCA808X_LED_FORCE_EN +#define QCA807X_GPIO_FORCE_MODE_MASK QCA808X_LED_FORCE_MODE_MASK #define QCA807X_FUNCTION_CONTROL 0x10 #define QCA807X_FC_MDI_CROSSOVER_MODE_MASK GENMASK(6, 5) @@ -105,6 +115,233 @@ static int qca807x_cable_test_start(struct phy_device *phydev) return 0; } +static int qca807x_led_parse_netdev(struct phy_device *phydev, unsigned long rules, + u16 *offload_trigger) +{ + /* Parsing specific to netdev trigger */ + switch (phydev->port) { + case PORT_TP: + if (test_bit(TRIGGER_NETDEV_TX, &rules)) + *offload_trigger |= QCA808X_LED_TX_BLINK; + if (test_bit(TRIGGER_NETDEV_RX, &rules)) + *offload_trigger |= QCA808X_LED_RX_BLINK; + if (test_bit(TRIGGER_NETDEV_LINK_10, &rules)) + *offload_trigger |= QCA808X_LED_SPEED10_ON; + if (test_bit(TRIGGER_NETDEV_LINK_100, &rules)) + *offload_trigger |= QCA808X_LED_SPEED100_ON; + if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules)) + *offload_trigger |= QCA808X_LED_SPEED1000_ON; + if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules)) + *offload_trigger |= QCA808X_LED_HALF_DUPLEX_ON; + if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules)) + *offload_trigger |= QCA808X_LED_FULL_DUPLEX_ON; + break; + case PORT_FIBRE: + if (test_bit(TRIGGER_NETDEV_TX, &rules)) + *offload_trigger |= QCA807X_LED_FIBER_TXACT_BLK_EN; + if (test_bit(TRIGGER_NETDEV_RX, &rules)) + *offload_trigger |= QCA807X_LED_FIBER_RXACT_BLK_EN; + if (test_bit(TRIGGER_NETDEV_LINK_100, &rules)) + *offload_trigger |= QCA807X_LED_FIBER_100FX_ON_EN; + if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules)) + *offload_trigger |= QCA807X_LED_FIBER_1000BX_ON_EN; + if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules)) + *offload_trigger |= QCA807X_LED_FIBER_HDX_ON_EN; + if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules)) + *offload_trigger |= QCA807X_LED_FIBER_FDX_ON_EN; + break; + default: + return -EOPNOTSUPP; + } + + if (rules && !*offload_trigger) + return -EOPNOTSUPP; + + return 0; +} + +static int qca807x_led_hw_control_enable(struct phy_device *phydev, u8 index) +{ + u16 reg; + + if (index > 1) + return -EINVAL; + + reg = QCA807X_MMD7_LED_FORCE_CTRL(index); + return qca808x_led_reg_hw_control_enable(phydev, reg); +} + +static int qca807x_led_hw_is_supported(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + u16 offload_trigger = 0; + + if (index > 1) + return -EINVAL; + + return qca807x_led_parse_netdev(phydev, rules, &offload_trigger); +} + +static int qca807x_led_hw_control_set(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + u16 reg, mask, offload_trigger = 0; + int ret; + + if (index > 1) + return -EINVAL; + + ret = qca807x_led_parse_netdev(phydev, rules, &offload_trigger); + if (ret) + return ret; + + ret = qca807x_led_hw_control_enable(phydev, index); + if (ret) + return ret; + + switch (phydev->port) { + case PORT_TP: + reg = QCA807X_MMD7_LED_CTRL(index); + mask = QCA808X_LED_PATTERN_MASK; + break; + case PORT_FIBRE: + /* HW control pattern bits are in LED FORCE reg */ + reg = QCA807X_MMD7_LED_FORCE_CTRL(index); + mask = QCA807X_LED_FIBER_PATTERN_MASK; + break; + default: + return -EINVAL; + } + + return phy_modify_mmd(phydev, MDIO_MMD_AN, reg, mask, + offload_trigger); +} + +static bool qca807x_led_hw_control_status(struct phy_device *phydev, u8 index) +{ + u16 reg; + + if (index > 1) + return false; + + reg = QCA807X_MMD7_LED_FORCE_CTRL(index); + return qca808x_led_reg_hw_control_status(phydev, reg); +} + +static int qca807x_led_hw_control_get(struct phy_device *phydev, u8 index, + unsigned long *rules) +{ + u16 reg; + int val; + + if (index > 1) + return -EINVAL; + + /* Check if we have hw control enabled */ + if (qca807x_led_hw_control_status(phydev, index)) + return -EINVAL; + + /* Parsing specific to netdev trigger */ + switch (phydev->port) { + case PORT_TP: + reg = QCA807X_MMD7_LED_CTRL(index); + val = phy_read_mmd(phydev, MDIO_MMD_AN, reg); + if (val & QCA808X_LED_TX_BLINK) + set_bit(TRIGGER_NETDEV_TX, rules); + if (val & QCA808X_LED_RX_BLINK) + set_bit(TRIGGER_NETDEV_RX, rules); + if (val & QCA808X_LED_SPEED10_ON) + set_bit(TRIGGER_NETDEV_LINK_10, rules); + if (val & QCA808X_LED_SPEED100_ON) + set_bit(TRIGGER_NETDEV_LINK_100, rules); + if (val & QCA808X_LED_SPEED1000_ON) + set_bit(TRIGGER_NETDEV_LINK_1000, rules); + if (val & QCA808X_LED_HALF_DUPLEX_ON) + set_bit(TRIGGER_NETDEV_HALF_DUPLEX, rules); + if (val & QCA808X_LED_FULL_DUPLEX_ON) + set_bit(TRIGGER_NETDEV_FULL_DUPLEX, rules); + break; + case PORT_FIBRE: + /* HW control pattern bits are in LED FORCE reg */ + reg = QCA807X_MMD7_LED_FORCE_CTRL(index); + val = phy_read_mmd(phydev, MDIO_MMD_AN, reg); + if (val & QCA807X_LED_FIBER_TXACT_BLK_EN) + set_bit(TRIGGER_NETDEV_TX, rules); + if (val & QCA807X_LED_FIBER_RXACT_BLK_EN) + set_bit(TRIGGER_NETDEV_RX, rules); + if (val & QCA807X_LED_FIBER_100FX_ON_EN) + set_bit(TRIGGER_NETDEV_LINK_100, rules); + if (val & QCA807X_LED_FIBER_1000BX_ON_EN) + set_bit(TRIGGER_NETDEV_LINK_1000, rules); + if (val & QCA807X_LED_FIBER_HDX_ON_EN) + set_bit(TRIGGER_NETDEV_HALF_DUPLEX, rules); + if (val & QCA807X_LED_FIBER_FDX_ON_EN) + set_bit(TRIGGER_NETDEV_FULL_DUPLEX, rules); + break; + default: + return -EINVAL; + } + + return 0; +} + +static int qca807x_led_hw_control_reset(struct phy_device *phydev, u8 index) +{ + u16 reg, mask; + + if (index > 1) + return -EINVAL; + + switch (phydev->port) { + case PORT_TP: + reg = QCA807X_MMD7_LED_CTRL(index); + mask = QCA808X_LED_PATTERN_MASK; + break; + case PORT_FIBRE: + /* HW control pattern bits are in LED FORCE reg */ + reg = QCA807X_MMD7_LED_FORCE_CTRL(index); + mask = QCA807X_LED_FIBER_PATTERN_MASK; + break; + default: + return -EINVAL; + } + + return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, reg, mask); +} + +static int qca807x_led_brightness_set(struct phy_device *phydev, + u8 index, enum led_brightness value) +{ + u16 reg; + int ret; + + if (index > 1) + return -EINVAL; + + /* If we are setting off the LED reset any hw control rule */ + if (!value) { + ret = qca807x_led_hw_control_reset(phydev, index); + if (ret) + return ret; + } + + reg = QCA807X_MMD7_LED_FORCE_CTRL(index); + return qca808x_led_reg_brightness_set(phydev, reg, value); +} + +static int qca807x_led_blink_set(struct phy_device *phydev, u8 index, + unsigned long *delay_on, + unsigned long *delay_off) +{ + u16 reg; + + if (index > 1) + return -EINVAL; + + reg = QCA807X_MMD7_LED_FORCE_CTRL(index); + return qca808x_led_reg_blink_set(phydev, reg, delay_on, delay_off); +} + #ifdef CONFIG_GPIOLIB static int qca807x_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) { @@ -481,11 +718,23 @@ static int qca807x_probe(struct phy_device *phydev) return -EINVAL; if (IS_ENABLED(CONFIG_GPIOLIB)) { + if (of_find_property(node, "leds", NULL) && + of_find_property(node, "gpio-controller", NULL)) { + phydev_err(phydev, "Invalid property detected. LEDs and gpio-controller are mutually exclusive."); + return -EINVAL; + } + /* Do not register a GPIO controller unless flagged for it */ if (of_property_read_bool(node, "gpio-controller")) { ret = qca807x_gpio(phydev); if (ret) return ret; + + phydev->drv->led_brightness_set = NULL; + phydev->drv->led_blink_set = NULL; + phydev->drv->led_hw_is_supported = NULL; + phydev->drv->led_hw_control_set = NULL; + phydev->drv->led_hw_control_get = NULL; } } @@ -561,6 +810,11 @@ static struct phy_driver qca807x_drivers[] = { .suspend = genphy_suspend, .cable_test_start = qca807x_cable_test_start, .cable_test_get_status = qca808x_cable_test_get_status, + .led_brightness_set = qca807x_led_brightness_set, + .led_blink_set = qca807x_led_blink_set, + .led_hw_is_supported = qca807x_led_hw_is_supported, + .led_hw_control_set = qca807x_led_hw_control_set, + .led_hw_control_get = qca807x_led_hw_control_get, }, }; module_phy_driver(qca807x_drivers);
QCA8072/5 have up to 2 LEDs attached for PHY. LEDs can be configured to be ON/hw blink or be set to HW control. Hw blink mode is set to blink at 4Hz or 250ms. PHY can support both copper (TP) or fiber (FIBRE) kind and supports different HW control modes based on the port type. HW control modes supported for netdev trigger for copper ports are: - LINK_10 - LINK_100 - LINK_1000 - TX - RX - FULL_DUPLEX - HALF_DUPLEX HW control modes supported for netdev trigger for fiber ports are: - LINK_100 - LINK_1000 - TX - RX - FULL_DUPLEX - HALF_DUPLEX LED support conflicts with GPIO controller feature and must be disabled if gpio-controller is used for the PHY. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/net/phy/qcom/qca807x.c | 258 ++++++++++++++++++++++++++++++++- 1 file changed, 256 insertions(+), 2 deletions(-)