| Message ID | 20240227093945.21525-4-bastien.curutchet@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: Add TI's DP83640 device tree binding | expand |
Hello Bastien, On Tue, 27 Feb 2024 10:39:42 +0100 Bastien Curutchet <bastien.curutchet@bootlin.com> wrote: > The PHY have three LED : Activity LED, Speed LED and Link LED. The PHY > offers three configuration modes for them. The driver does not handle them. > > Add LED handling through the /sys/class/leds interface. > On every mode the Speed LED indicates the speed (10Mbps or 100 Mbps) and > the Link LED indicates whether the link is good or not. Link LED will also > reflects link activity in mode 2 and 3. The Activity LED can reflect the > link activity (mode 1), the link's duplex (mode 2) or collisions on the > link (mode 3). > Only the Activity LED can have its hardware configuration updated, it > has an impact on Link LED as activity reflexion is added on it on modes > 2 and 3 > > Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com> Regarding the LED management I don't have much knowledge on it, however I do have comments on the code itself :) > --- > drivers/net/phy/dp83640.c | 176 ++++++++++++++++++++++++++++++++++ > drivers/net/phy/dp83640_reg.h | 11 +++ > 2 files changed, 187 insertions(+) > > diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c > index 5c42c47dc564..c46c81ef0ad0 100644 > --- a/drivers/net/phy/dp83640.c > +++ b/drivers/net/phy/dp83640.c > @@ -59,6 +59,22 @@ > MII_DP83640_MISR_SPD_INT |\ > MII_DP83640_MISR_LINK_INT) > > +#define DP83640_ACTLED_IDX 0 > +#define DP83640_LNKLED_IDX 1 > +#define DP83640_SPDLED_IDX 2 > +/* LNKLED = ON for Good Link, OFF for No Link */ > +/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */ > +/* ACTLED = ON for Activity, OFF for No Activity */ > +#define DP83640_LED_MODE_1 1 > +/* LNKLED = ON for Good Link, Blink for Activity */ > +/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */ > +/* ACTLED = ON for Collision, OFF for No Collision */ > +#define DP83640_LED_MODE_2 2 > +/* LNKLED = ON for Good Link, Blink for Activity */ > +/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */ > +/* ACTLED = ON for Full-Duplex, OFF for Half-Duplex */ > +#define DP83640_LED_MODE_3 3 > + > /* phyter seems to miss the mark by 16 ns */ > #define ADJTIME_FIX 16 > > @@ -1515,6 +1531,161 @@ static void dp83640_remove(struct phy_device *phydev) > kfree(dp83640); > } > > +static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index, > + enum led_brightness brightness) > +{ > + int val; > + > + if (index > DP83640_SPDLED_IDX) > + return -EINVAL; > + > + phy_write(phydev, PAGESEL, 0); > + > + val = phy_read(phydev, LEDCR) & ~DP83640_LED_DIS(index); > + val |= DP83640_LED_DRV(index); > + if (brightness) > + val |= DP83640_LED_VAL(index); > + else > + val &= ~DP83640_LED_VAL(index); > + phy_write(phydev, LEDCR, val); Looks like you can use phy_modify here > + > + return 0; > +} > + > +/** > + * dp83640_led_mode - Check the trigger netdev rules and compute the associated > + * configuration mode > + * @index: The targeted LED > + * @rules: Rules to be checked > + * > + * Returns the mode that is to be set in LED_CFNG. If the rules are not > + * supported by the PHY, returns -ENOTSUPP. If the rules are supported but don't > + * impact the LED configuration, returns 0 > + */ > +static int dp83640_led_mode(u8 index, unsigned long rules) > +{ > + /* Only changes on ACTLED have an impact on LED Mode configuration */ > + switch (index) { > + case DP83640_ACTLED_IDX: > + switch (rules) { > + case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): > + return DP83640_LED_MODE_1; > + case BIT(TRIGGER_NETDEV_COLLISION): > + return DP83640_LED_MODE_2; > + case BIT(TRIGGER_NETDEV_FULL_DUPLEX) | > + BIT(TRIGGER_NETDEV_HALF_DUPLEX): > + return DP83640_LED_MODE_3; > + default: > + return -EOPNOTSUPP; > + } > + > + case DP83640_SPDLED_IDX: > + /* SPDLED has the same function in every mode */ > + switch (rules) { > + case BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK_100): > + return 0; > + default: > + return -EOPNOTSUPP; > + } > + > + case DP83640_LNKLED_IDX: > + /* LNKLED has the same function in every mode */ > + switch (rules) { > + case BIT(TRIGGER_NETDEV_LINK): > + return 0; > + default: > + return -EOPNOTSUPP; > + } > + default: > + return -EINVAL; > + } > +} > + > +static int dp83640_led_hw_is_supported(struct phy_device *phydev, u8 index, > + unsigned long rules) > +{ > + int ret; > + > + ret = dp83640_led_mode(index, rules); > + > + if (ret < 0) > + return ret; > + > + return 0; > +} It looks like you can just return whatever dp83640_led_mode() returns directly ? > + > +static int dp83640_led_hw_control_set(struct phy_device *phydev, u8 index, > + unsigned long rules) > +{ > + int mode, val; > + > + mode = dp83640_led_mode(index, rules); > + if (mode < 0) > + return mode; > + > + if (mode) { > + phy_write(phydev, PAGESEL, 0); Here the current page is written to 0, don't you need to restore the page to the original one afterwards ? the broadcast_* functions in the same driver are always restoring the page to the initial one, I think you also need that, or unrelated register accesses in the same PHY driver might write to the wrong page. If you want that to be done automatically for you, you can implement the .read_page and .write_page in the driver, then use the phy_read/write/modify_paged accessors. > + val = phy_read(phydev, PHYCR) & ~(LED_CNFG_1 | LED_CNFG_0); > + switch (mode) { > + case DP83640_LED_MODE_1: > + val |= LED_CNFG_0; > + break; > + case DP83640_LED_MODE_2: > + /* Keeping LED_CNFG_1 and LED_CNFG_0 unset */ > + break; > + case DP83640_LED_MODE_3: > + val |= LED_CNFG_1; > + break; > + default: > + return -EINVAL; > + } > + phy_write(phydev, PHYCR, val); > + } > + > + val = phy_read(phydev, LEDCR); > + val &= ~(DP83640_LED_DIS(index) | DP83640_LED_DRV(index)); > + phy_write(phydev, LEDCR, val); You can use phy_modify here aswell > + > + return 0; > +} > + > +static int dp83640_led_hw_control_get(struct phy_device *phydev, u8 index, > + unsigned long *rules) > +{ > + int val; > + > + switch (index) { > + case DP83640_ACTLED_IDX: > + phy_write(phydev, PAGESEL, 0); Same comment on the page selection here. > + val = phy_read(phydev, PHYCR); > + if (val & LED_CNFG_0) { > + /* Mode 1 */ > + *rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX); > + } else if (val & LED_CNFG_1) { > + /* Mode 3 */ > + *rules = BIT(TRIGGER_NETDEV_FULL_DUPLEX) | > + BIT(TRIGGER_NETDEV_HALF_DUPLEX); > + } else { > + /* Mode 2 */ > + *rules = BIT(TRIGGER_NETDEV_COLLISION); > + } > + break; > + > + case DP83640_LNKLED_IDX: > + *rules = BIT(TRIGGER_NETDEV_LINK); > + break; > + > + case DP83640_SPDLED_IDX: > + *rules = BIT(TRIGGER_NETDEV_LINK_10) | > + BIT(TRIGGER_NETDEV_LINK_100); > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > static struct phy_driver dp83640_driver = { > .phy_id = DP83640_PHY_ID, > .phy_id_mask = 0xfffffff0, > @@ -1526,6 +1697,11 @@ static struct phy_driver dp83640_driver = { > .config_init = dp83640_config_init, > .config_intr = dp83640_config_intr, > .handle_interrupt = dp83640_handle_interrupt, > + > + .led_brightness_set = dp83640_led_brightness_set, > + .led_hw_is_supported = dp83640_led_hw_is_supported, > + .led_hw_control_set = dp83640_led_hw_control_set, > + .led_hw_control_get = dp83640_led_hw_control_get, > }; > > static int __init dp83640_init(void) > diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h > index daae7fa58fb8..09dd2d2527c7 100644 > --- a/drivers/net/phy/dp83640_reg.h > +++ b/drivers/net/phy/dp83640_reg.h > @@ -6,6 +6,8 @@ > #define HAVE_DP83640_REGISTERS > > /* #define PAGE0 0x0000 */ > +#define LEDCR 0x0018 /* PHY Control Register */ > +#define PHYCR 0x0019 /* PHY Control Register */ > #define PHYCR2 0x001c /* PHY Control Register 2 */ > > #define PAGE4 0x0004 > @@ -50,6 +52,15 @@ > #define PTP_GPIOMON 0x001e /* PTP GPIO Monitor Register */ > #define PTP_RXHASH 0x001f /* PTP Receive Hash Register */ > > +/* Bit definitions for the LEDCR register */ > +#define DP83640_LED_DIS(x) BIT((x) + 9) /* Disable LED */ > +#define DP83640_LED_DRV(x) BIT((x) + 3) /* Force LED val to output */ > +#define DP83640_LED_VAL(x) BIT((x)) /* LED val */ > + > +/* Bit definitions for the PHYCR register */ > +#define LED_CNFG_0 BIT(5) /* LED configuration, bit 0 */ > +#define LED_CNFG_1 BIT(6) /* LED configuration, bit 1 */ > + > /* Bit definitions for the PHYCR2 register */ > #define BC_WRITE (1<<11) /* Broadcast Write Enable */ > Thanks, Maxime
On Tue, Feb 27, 2024 at 10:58:06AM +0100, Maxime Chevallier wrote: > > + val = phy_read(phydev, PHYCR) & ~(LED_CNFG_1 | LED_CNFG_0); > > + switch (mode) { > > + case DP83640_LED_MODE_1: > > + val |= LED_CNFG_0; > > + break; > > + case DP83640_LED_MODE_2: > > + /* Keeping LED_CNFG_1 and LED_CNFG_0 unset */ > > + break; > > + case DP83640_LED_MODE_3: > > + val |= LED_CNFG_1; > > + break; > > + default: > > + return -EINVAL; > > + } > > + phy_write(phydev, PHYCR, val); This should also be phy_modify() as well. Any read-modify-write sequence is open to race conditions if it is open coded because the bus lock will be dropped after the read and regained on the write.
> +static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index, > + enum led_brightness brightness) > +{ > + int val; > + > + if (index > DP83640_SPDLED_IDX) > + return -EINVAL; > + > + phy_write(phydev, PAGESEL, 0); > + > + val = phy_read(phydev, LEDCR) & ~DP83640_LED_DIS(index); > + val |= DP83640_LED_DRV(index); > + if (brightness) > + val |= DP83640_LED_VAL(index); > + else > + val &= ~DP83640_LED_VAL(index); > + phy_write(phydev, LEDCR, val); I don't understand this driver too well, but should this be using ext_read() and ext_write(). I'm also woundering if it would be good to implement the .read_page and .write_page members in phy_driver and then use phy_write_paged() and phy_write_paged() and phy_modify_paged(), which should do better locking. Andrew
Hi On 2/28/24 16:04, Andrew Lunn wrote: >> +static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index, >> + enum led_brightness brightness) >> +{ >> + int val; >> + >> + if (index > DP83640_SPDLED_IDX) >> + return -EINVAL; >> + >> + phy_write(phydev, PAGESEL, 0); >> + >> + val = phy_read(phydev, LEDCR) & ~DP83640_LED_DIS(index); >> + val |= DP83640_LED_DRV(index); >> + if (brightness) >> + val |= DP83640_LED_VAL(index); >> + else >> + val &= ~DP83640_LED_VAL(index); >> + phy_write(phydev, LEDCR, val); > I don't understand this driver too well, but should this be using > ext_read() and ext_write(). > > I'm also woundering if it would be good to implement the .read_page > and .write_page members in phy_driver and then use phy_write_paged() > and phy_write_paged() and phy_modify_paged(), which should do better > locking. I don't feel comfortable implementing .read_page and write_page members as I have only one PHY on my board so I'll not be able to test all the broadcast thing. If that's OK with you, I'll use the ext_read() and ext_write() Best regards, Bastien
diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c index 5c42c47dc564..c46c81ef0ad0 100644 --- a/drivers/net/phy/dp83640.c +++ b/drivers/net/phy/dp83640.c @@ -59,6 +59,22 @@ MII_DP83640_MISR_SPD_INT |\ MII_DP83640_MISR_LINK_INT) +#define DP83640_ACTLED_IDX 0 +#define DP83640_LNKLED_IDX 1 +#define DP83640_SPDLED_IDX 2 +/* LNKLED = ON for Good Link, OFF for No Link */ +/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */ +/* ACTLED = ON for Activity, OFF for No Activity */ +#define DP83640_LED_MODE_1 1 +/* LNKLED = ON for Good Link, Blink for Activity */ +/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */ +/* ACTLED = ON for Collision, OFF for No Collision */ +#define DP83640_LED_MODE_2 2 +/* LNKLED = ON for Good Link, Blink for Activity */ +/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */ +/* ACTLED = ON for Full-Duplex, OFF for Half-Duplex */ +#define DP83640_LED_MODE_3 3 + /* phyter seems to miss the mark by 16 ns */ #define ADJTIME_FIX 16 @@ -1515,6 +1531,161 @@ static void dp83640_remove(struct phy_device *phydev) kfree(dp83640); } +static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index, + enum led_brightness brightness) +{ + int val; + + if (index > DP83640_SPDLED_IDX) + return -EINVAL; + + phy_write(phydev, PAGESEL, 0); + + val = phy_read(phydev, LEDCR) & ~DP83640_LED_DIS(index); + val |= DP83640_LED_DRV(index); + if (brightness) + val |= DP83640_LED_VAL(index); + else + val &= ~DP83640_LED_VAL(index); + phy_write(phydev, LEDCR, val); + + return 0; +} + +/** + * dp83640_led_mode - Check the trigger netdev rules and compute the associated + * configuration mode + * @index: The targeted LED + * @rules: Rules to be checked + * + * Returns the mode that is to be set in LED_CFNG. If the rules are not + * supported by the PHY, returns -ENOTSUPP. If the rules are supported but don't + * impact the LED configuration, returns 0 + */ +static int dp83640_led_mode(u8 index, unsigned long rules) +{ + /* Only changes on ACTLED have an impact on LED Mode configuration */ + switch (index) { + case DP83640_ACTLED_IDX: + switch (rules) { + case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): + return DP83640_LED_MODE_1; + case BIT(TRIGGER_NETDEV_COLLISION): + return DP83640_LED_MODE_2; + case BIT(TRIGGER_NETDEV_FULL_DUPLEX) | + BIT(TRIGGER_NETDEV_HALF_DUPLEX): + return DP83640_LED_MODE_3; + default: + return -EOPNOTSUPP; + } + + case DP83640_SPDLED_IDX: + /* SPDLED has the same function in every mode */ + switch (rules) { + case BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK_100): + return 0; + default: + return -EOPNOTSUPP; + } + + case DP83640_LNKLED_IDX: + /* LNKLED has the same function in every mode */ + switch (rules) { + case BIT(TRIGGER_NETDEV_LINK): + return 0; + default: + return -EOPNOTSUPP; + } + default: + return -EINVAL; + } +} + +static int dp83640_led_hw_is_supported(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + int ret; + + ret = dp83640_led_mode(index, rules); + + if (ret < 0) + return ret; + + return 0; +} + +static int dp83640_led_hw_control_set(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + int mode, val; + + mode = dp83640_led_mode(index, rules); + if (mode < 0) + return mode; + + if (mode) { + phy_write(phydev, PAGESEL, 0); + val = phy_read(phydev, PHYCR) & ~(LED_CNFG_1 | LED_CNFG_0); + switch (mode) { + case DP83640_LED_MODE_1: + val |= LED_CNFG_0; + break; + case DP83640_LED_MODE_2: + /* Keeping LED_CNFG_1 and LED_CNFG_0 unset */ + break; + case DP83640_LED_MODE_3: + val |= LED_CNFG_1; + break; + default: + return -EINVAL; + } + phy_write(phydev, PHYCR, val); + } + + val = phy_read(phydev, LEDCR); + val &= ~(DP83640_LED_DIS(index) | DP83640_LED_DRV(index)); + phy_write(phydev, LEDCR, val); + + return 0; +} + +static int dp83640_led_hw_control_get(struct phy_device *phydev, u8 index, + unsigned long *rules) +{ + int val; + + switch (index) { + case DP83640_ACTLED_IDX: + phy_write(phydev, PAGESEL, 0); + val = phy_read(phydev, PHYCR); + if (val & LED_CNFG_0) { + /* Mode 1 */ + *rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX); + } else if (val & LED_CNFG_1) { + /* Mode 3 */ + *rules = BIT(TRIGGER_NETDEV_FULL_DUPLEX) | + BIT(TRIGGER_NETDEV_HALF_DUPLEX); + } else { + /* Mode 2 */ + *rules = BIT(TRIGGER_NETDEV_COLLISION); + } + break; + + case DP83640_LNKLED_IDX: + *rules = BIT(TRIGGER_NETDEV_LINK); + break; + + case DP83640_SPDLED_IDX: + *rules = BIT(TRIGGER_NETDEV_LINK_10) | + BIT(TRIGGER_NETDEV_LINK_100); + break; + default: + return -EINVAL; + } + + return 0; +} + static struct phy_driver dp83640_driver = { .phy_id = DP83640_PHY_ID, .phy_id_mask = 0xfffffff0, @@ -1526,6 +1697,11 @@ static struct phy_driver dp83640_driver = { .config_init = dp83640_config_init, .config_intr = dp83640_config_intr, .handle_interrupt = dp83640_handle_interrupt, + + .led_brightness_set = dp83640_led_brightness_set, + .led_hw_is_supported = dp83640_led_hw_is_supported, + .led_hw_control_set = dp83640_led_hw_control_set, + .led_hw_control_get = dp83640_led_hw_control_get, }; static int __init dp83640_init(void) diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h index daae7fa58fb8..09dd2d2527c7 100644 --- a/drivers/net/phy/dp83640_reg.h +++ b/drivers/net/phy/dp83640_reg.h @@ -6,6 +6,8 @@ #define HAVE_DP83640_REGISTERS /* #define PAGE0 0x0000 */ +#define LEDCR 0x0018 /* PHY Control Register */ +#define PHYCR 0x0019 /* PHY Control Register */ #define PHYCR2 0x001c /* PHY Control Register 2 */ #define PAGE4 0x0004 @@ -50,6 +52,15 @@ #define PTP_GPIOMON 0x001e /* PTP GPIO Monitor Register */ #define PTP_RXHASH 0x001f /* PTP Receive Hash Register */ +/* Bit definitions for the LEDCR register */ +#define DP83640_LED_DIS(x) BIT((x) + 9) /* Disable LED */ +#define DP83640_LED_DRV(x) BIT((x) + 3) /* Force LED val to output */ +#define DP83640_LED_VAL(x) BIT((x)) /* LED val */ + +/* Bit definitions for the PHYCR register */ +#define LED_CNFG_0 BIT(5) /* LED configuration, bit 0 */ +#define LED_CNFG_1 BIT(6) /* LED configuration, bit 1 */ + /* Bit definitions for the PHYCR2 register */ #define BC_WRITE (1<<11) /* Broadcast Write Enable */
The PHY have three LED : Activity LED, Speed LED and Link LED. The PHY offers three configuration modes for them. The driver does not handle them. Add LED handling through the /sys/class/leds interface. On every mode the Speed LED indicates the speed (10Mbps or 100 Mbps) and the Link LED indicates whether the link is good or not. Link LED will also reflects link activity in mode 2 and 3. The Activity LED can reflect the link activity (mode 1), the link's duplex (mode 2) or collisions on the link (mode 3). Only the Activity LED can have its hardware configuration updated, it has an impact on Link LED as activity reflexion is added on it on modes 2 and 3 Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com> --- drivers/net/phy/dp83640.c | 176 ++++++++++++++++++++++++++++++++++ drivers/net/phy/dp83640_reg.h | 11 +++ 2 files changed, 187 insertions(+)