From b977f87bd0d1d0028cdfa18519cbb5daf53c2015 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fin=20Maa=C3=9F?= Date: Wed, 18 Jun 2025 12:07:22 +0200 Subject: [PATCH 1/9] drivers: ethernet: phy: microchip_vsc8541: improve driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - implement configure link - support half duplex - use defines from mii.h - fix check ret vals Signed-off-by: Fin Maaß Signed-off-by: Fin Maaß --- drivers/ethernet/phy/phy_microchip_vsc8541.c | 187 +++++++------------ 1 file changed, 70 insertions(+), 117 deletions(-) diff --git a/drivers/ethernet/phy/phy_microchip_vsc8541.c b/drivers/ethernet/phy/phy_microchip_vsc8541.c index d30895feb9a9..2961e44e67d3 100644 --- a/drivers/ethernet/phy/phy_microchip_vsc8541.c +++ b/drivers/ethernet/phy/phy_microchip_vsc8541.c @@ -16,6 +16,8 @@ LOG_MODULE_REGISTER(microchip_vsc8541, CONFIG_PHY_LOG_LEVEL); #include #include +#include "phy_mii.h" + /* phy page selectors */ #define PHY_PAGE_0 0x00 /* main registers space active */ #define PHY_PAGE_1 0x01 /* reg 16 - 30 will be redirected to ext. register space 1 */ @@ -26,18 +28,6 @@ LOG_MODULE_REGISTER(microchip_vsc8541, CONFIG_PHY_LOG_LEVEL); #define PHY_REG(page, reg) ((page << 8) | (reg << 0)) /* Generic Register */ -#define PHY_REG_PAGE0_BMCR PHY_REG(PHY_PAGE_0, 0x00) -#define PHY_REG_PAGE0_BMSR PHY_REG(PHY_PAGE_0, 0x01) -#define PHY_REG_PAGE0_ID1 PHY_REG(PHY_PAGE_0, 0x02) -#define PHY_REG_PAGE0_ID2 PHY_REG(PHY_PAGE_0, 0x03) -#define PHY_REG_PAGE0_ADV PHY_REG(PHY_PAGE_0, 0x04) -#define PHY_REG_LPA 0x05 -#define PHY_REG_EXP 0x06 -#define PHY_REG_PAGE0_CTRL1000 PHY_REG(PHY_PAGE_0, 0x09) -#define PHY_REG_PAGE0_STAT1000 PHY_REG(0, 0x0A) -#define PHY_REG_MMD_CTRL 0x0D -#define PHY_REG_MMD_DATA 0x0E -#define PHY_REG_STAT1000_EXT1 0x0F #define PHY_REG_PAGE0_STAT100 PHY_REG(PHY_PAGE_0, 0x10) #define PHY_REG_PAGE0_STAT1000_EXT2 PHY_REG(PHY_PAGE_0, 0x11) #define PHY_REG_AUX_CTRL 0x12 @@ -45,6 +35,7 @@ LOG_MODULE_REGISTER(microchip_vsc8541, CONFIG_PHY_LOG_LEVEL); #define PHY_REG_PAGE0_ERROR_COUNTER_2 PHY_REG(0, 0x14) #define PHY_REG_PAGE0_EXT_CTRL_STAT PHY_REG(PHY_PAGE_0, 0x16) #define PHY_REG_PAGE0_EXT_CONTROL_1 PHY_REG(PHY_PAGE_0, 0x17) +#define PHY_REG_PAGE0_EXT_DEV_AUX PHY_REG(PHY_PAGE_0, 0x1C) #define PHY_REG_LED_MODE 0x1d #define PHY_REG_PAGE_SELECTOR 0x1F @@ -54,19 +45,7 @@ LOG_MODULE_REGISTER(microchip_vsc8541, CONFIG_PHY_LOG_LEVEL); #define PHY_REG_PAGE2_RGMII_CONTROL PHY_REG(PHY_PAGE_2, 0x14) #define PHY_REG_PAGE2_MAC_IF_CONTROL PHY_REG(PHY_PAGE_2, 0x1b) -/* selected bits in registers */ -#define BMCR_RESET (1 << 15) -#define BMCR_LOOPBACK (1 << 14) -#define BMCR_ANENABLE (1 << 12) -#define BMCR_ANRESTART (1 << 9) -#define BMCR_FULLDPLX (1 << 8) -#define BMCR_SPEED10 ((0 << 13) | (0 << 6)) -#define BMCR_SPEED100 ((1 << 13) | (0 << 6)) -#define BMCR_SPEED1000 ((0 << 13) | (1 << 6)) - -#define BMCR_SPEEDMASK ((1 << 13) | (1 << 6)) - -#define BMSR_LSTATUS (1 << 2) +#define PHY_REG_PAGE0_EXT_DEV_AUX_DUPLEX BIT(5) enum vsc8541_interface { VSC8541_MII, @@ -123,11 +102,11 @@ static int phy_mc_vsc8541_verify_phy_id(const struct device *dev) uint16_t phy_id_1; uint16_t phy_id_2; - if (0 != phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_ID1, (uint32_t *)&phy_id_1)) { + if (phy_mc_vsc8541_read(dev, MII_PHYID1R, (uint32_t *)&phy_id_1) < 0) { return -EINVAL; } - if (0 != phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_ID2, (uint32_t *)&phy_id_2)) { + if (phy_mc_vsc8541_read(dev, MII_PHYID2R, (uint32_t *)&phy_id_2) < 0) { return -EINVAL; } @@ -153,6 +132,8 @@ static int phy_mc_vsc8541_verify_phy_id(const struct device *dev) static int phy_mc_vsc8541_reset(const struct device *dev) { const struct mc_vsc8541_config *cfg = dev->config; + int ret; + uint32_t reg = 0U; #if DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios) @@ -162,16 +143,15 @@ static int phy_mc_vsc8541_reset(const struct device *dev) } /* configure the reset pin */ - int ret = gpio_pin_configure_dt(&cfg->reset_gpio, GPIO_OUTPUT_ACTIVE); - - if (ret) { + ret = gpio_pin_configure_dt(&cfg->reset_gpio, GPIO_OUTPUT_ACTIVE); + if (ret < 0) { return ret; } for (uint32_t i = 0; i < 2; i++) { /* Start reset */ ret = gpio_pin_set_dt(&cfg->reset_gpio, 0); - if (ret) { + if (ret < 0) { LOG_WRN("failed to set reset gpio"); return -EINVAL; } @@ -194,7 +174,7 @@ static int phy_mc_vsc8541_reset(const struct device *dev) #if CONFIG_PHY_VERIFY_DEVICE_IDENTIFICATION /* confirm phy organizationally unique identifier, if enabled */ - if (0 != phy_mc_vsc8541_verify_phy_id(dev)) { + if (phy_mc_vsc8541_verify_phy_id(dev) < 0) { LOG_ERR("failed to verify phy id"); return -EINVAL; } @@ -204,29 +184,21 @@ static int phy_mc_vsc8541_reset(const struct device *dev) if (cfg->microchip_interface_type == VSC8541_RGMII) { ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE0_EXT_CONTROL_1, (0x0 << 13) | (0x2 << 11)); - if (ret) { + if (ret < 0) { return ret; } } /* software reset */ - ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE0_BMCR, MII_BMCR_RESET); - if (ret) { + ret = phy_mc_vsc8541_write(dev, MII_BMCR, MII_BMCR_RESET); + if (ret < 0) { return ret; } /* wait for phy finished software reset */ - uint32_t reg = 0; - do { - phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_BMCR, ®); - } while (reg & BMCR_RESET); - - /* forced MDI-X */ - ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE1_EXT_MODE_CTRL, (3 << 2)); - if (ret) { - return ret; - } + phy_mc_vsc8541_read(dev, MII_BMCR, ®); + } while (reg & MII_BMCR_RESET); /* configure the RGMII clk delay */ reg = 0x0; @@ -234,32 +206,8 @@ static int phy_mc_vsc8541_reset(const struct device *dev) reg |= (cfg->rgmii_rx_clk_delay << 4); /* TX_CLK delay */ reg |= (cfg->rgmii_tx_clk_delay << 0); - ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE2_RGMII_CONTROL, reg); - if (ret) { - return ret; - } - - /* we use limited advertising, to force gigabit speed */ - /* initial version of this driver supports only 1GB/s */ - - /* 1000MBit/s + AUTO */ - ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE0_ADV, (1 << 8) | (1 << 6) | 0x01); - if (ret) { - return ret; - } - - ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE0_CTRL1000, (1 << 12) | (1 << 11) | (1 << 9)); - if (ret) { - return ret; - } - - /* start auto negotiation */ - ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE0_BMCR, BMCR_ANENABLE | BMCR_ANRESTART); - if (ret) { - return ret; - } - return ret; + return phy_mc_vsc8541_write(dev, PHY_REG_PAGE2_RGMII_CONTROL, reg); } /** @@ -271,54 +219,64 @@ static int phy_mc_vsc8541_reset(const struct device *dev) static int phy_mc_vsc8541_get_speed(const struct device *dev, struct phy_link_state *state) { int ret; - uint32_t status; + uint32_t aux_status; uint32_t link10_status; uint32_t link100_status; uint32_t link1000_status; + bool is_duplex; - ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_BMSR, &status); - if (ret) { + ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_EXT_DEV_AUX, &aux_status); + if (ret < 0) { return ret; } - ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_EXT_CTRL_STAT, &link10_status); - if (ret) { + is_duplex = (aux_status & PHY_REG_PAGE0_EXT_DEV_AUX_DUPLEX) != 0; + + ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_STAT1000_EXT2, &link1000_status); + if (ret < 0) { return ret; } - ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_STAT100, &link100_status); - if (ret) { - return ret; + if ((link1000_status & BIT(12))) { + state->speed = is_duplex ? LINK_FULL_1000BASE : LINK_HALF_1000BASE; + return 0; /* no need to check lower speeds */ } - ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_STAT1000_EXT2, &link1000_status); - if (ret) { + ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_STAT100, &link100_status); + if (ret < 0) { return ret; } - if ((status & (1 << 2)) == 0) { - /* no link */ - state->speed = LINK_HALF_10BASE; + if (link100_status & BIT(12)) { + state->speed = is_duplex ? LINK_FULL_100BASE : LINK_HALF_100BASE; + return 0; /* no need to check lower speeds */ } - if ((status & (1 << 5)) == 0) { - /* auto negotiation not yet complete */ - state->speed = LINK_HALF_10BASE; + ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_EXT_CTRL_STAT, &link10_status); + if (ret < 0) { + return ret; } - if ((link1000_status & (1 << 12))) { - state->speed = LINK_FULL_1000BASE; - } - if (link100_status & (1 << 12)) { - state->speed = LINK_FULL_100BASE; - } - if (link10_status & (1 << 6)) { - state->speed = LINK_FULL_10BASE; + if (link10_status & BIT(6)) { + state->speed = is_duplex ? LINK_FULL_10BASE : LINK_HALF_10BASE; + } else { + state->speed = 0; /* no link */ } return 0; } +static int phy_mc_vsc8541_cfg_link(const struct device *dev, enum phy_link_speed adv_speeds, + enum phy_cfg_link_flag flags) +{ + if (flags & PHY_FLAG_AUTO_NEGOTIATION_DISABLED) { + LOG_ERR("Disabling auto-negotiation is not supported by this driver"); + return -ENOTSUP; + } + + return phy_mii_cfg_link_autoneg(dev, adv_speeds, true); +} + /** * @brief Initializes the phy and starts the link monitor * @@ -326,17 +284,14 @@ static int phy_mc_vsc8541_get_speed(const struct device *dev, struct phy_link_st static int phy_mc_vsc8541_init(const struct device *dev) { struct mc_vsc8541_data *data = dev->data; + struct mc_vsc8541_config *cfg = dev->config; + int ret; - data->cb = NULL; - data->cb_data = NULL; - data->state.is_up = false; - data->state.speed = LINK_HALF_10BASE; data->active_page = -1; /* Reset PHY */ - int ret = phy_mc_vsc8541_reset(dev); - - if (ret) { + ret = phy_mc_vsc8541_reset(dev); + if (ret < 0) { LOG_ERR("initialize failed"); return ret; } @@ -363,37 +318,35 @@ static int phy_mc_vsc8541_get_link(const struct device *dev, struct phy_link_sta int ret; uint32_t reg_sr; uint32_t reg_cr; + bool hasLink; + bool auto_negotiation_finished = true; - ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_BMSR, ®_sr); - if (ret) { + ret = phy_mc_vsc8541_read(dev, MII_BMSR, ®_sr); + if (ret < 0) { return ret; } - ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_BMCR, ®_cr); - if (ret) { + ret = phy_mc_vsc8541_read(dev, MII_BMCR, ®_cr); + if (ret < 0) { return ret; } - uint32_t hasLink = reg_sr & (1 << 2) ? 1 : 0; + hasLink = (reg_sr & MII_BMSR_LINK_STATUS) != 0; - uint32_t auto_negotiation_finished; - - if (reg_cr & (BMCR_ANENABLE)) { + if (reg_cr & MII_BMCR_AUTONEG_ENABLE) { /* auto negotiation active; update status */ - auto_negotiation_finished = reg_sr & (1 << 5) ? 1 : 0; - } else { - auto_negotiation_finished = 1; + auto_negotiation_finished = (reg_sr & MII_BMSR_AUTONEG_COMPLETE) != 0; } - if (hasLink & auto_negotiation_finished) { - state->is_up = 1; + if (hasLink && auto_negotiation_finished) { + state->is_up = true; ret = phy_mc_vsc8541_get_speed(dev, state); - if (ret) { + if (ret < 0) { return ret; } } else { - state->is_up = 0; - state->speed = LINK_HALF_10BASE; + state->is_up = false; + state->speed = 0; } return 0; From c5fe7be40b6fc836d0d20bf1e905a3b7b9fd2d34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fin=20Maa=C3=9F?= Date: Wed, 18 Jun 2025 10:35:10 +0200 Subject: [PATCH 2/9] drivers: ethernet: phy: microchip_vsc8541: use mutex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit use mutex to protect page register phy_mc_vsc8541_get_link got removed from phy_mc_vsc8541_link_cb_set so, that phy_mc_vsc8541_link_monitor (own thread) is the only one to change data->state Signed-off-by: Fin Maaß --- drivers/ethernet/phy/phy_microchip_vsc8541.c | 32 +++++++++++--------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/drivers/ethernet/phy/phy_microchip_vsc8541.c b/drivers/ethernet/phy/phy_microchip_vsc8541.c index 2961e44e67d3..28d87cc39272 100644 --- a/drivers/ethernet/phy/phy_microchip_vsc8541.c +++ b/drivers/ethernet/phy/phy_microchip_vsc8541.c @@ -289,6 +289,8 @@ static int phy_mc_vsc8541_init(const struct device *dev) data->active_page = -1; + k_mutex_init(&data->mutex); + /* Reset PHY */ ret = phy_mc_vsc8541_reset(dev); if (ret < 0) { @@ -366,8 +368,6 @@ static int phy_mc_vsc8541_link_cb_set(const struct device *dev, phy_callback_t c data->cb = cb; data->cb_data = user_data; - phy_mc_vsc8541_get_link(dev, &data->state); - data->cb(dev, &data->state, data->cb_data); return 0; @@ -427,22 +427,24 @@ static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint /* mask out lower byte */ reg_addr &= 0x00ff; + k_mutex_lock(&dev_data->mutex, K_FOREVER); + /* select page, given by register upper byte */ if (dev_data->active_page != page) { ret = mdio_write(cfg->mdio_dev, cfg->addr, PHY_REG_PAGE_SELECTOR, (uint16_t)page); - if (ret) { - return ret; + if (ret < 0) { + goto read_end; } dev_data->active_page = (int)page; } /* select register, given by register lower byte */ ret = mdio_read(cfg->mdio_dev, cfg->addr, reg_addr, (uint16_t *)data); - if (ret) { - return ret; - } - return 0; +read_end: + k_mutex_unlock(&dev_data->mutex); + + return ret; } /** @@ -465,22 +467,24 @@ static int phy_mc_vsc8541_write(const struct device *dev, uint16_t reg_addr, uin /* mask out lower byte */ reg_addr &= 0x00ff; + k_mutex_lock(&dev_data->mutex, K_FOREVER); + /* select page, given by register upper byte */ if (dev_data->active_page != page) { ret = mdio_write(cfg->mdio_dev, cfg->addr, PHY_REG_PAGE_SELECTOR, (uint16_t)page); - if (ret) { - return ret; + if (ret < 0) { + goto write_end; } dev_data->active_page = (int)page; } /* write register, given by lower byte */ ret = mdio_write(cfg->mdio_dev, cfg->addr, reg_addr, (uint16_t)data); - if (ret) { - return ret; - } - return 0; +write_end: + k_mutex_unlock(&dev_data->mutex); + + return ret; } static DEVICE_API(ethphy, mc_vsc8541_phy_api) = { From 6ea6865adef0f38c0e9407b9b77d89d1db60aecc Mon Sep 17 00:00:00 2001 From: Robert Hancock Date: Wed, 18 Jun 2025 12:46:54 -0600 Subject: [PATCH 3/9] drivers: ethernet: phy: vsc8541: fixed build warnings Fixed some build warnings in the driver from previous changes by removing an unused variable and hooking up the cfg_link function. Also removes some implicit boolean conversions. Signed-off-by: Robert Hancock --- drivers/ethernet/phy/phy_microchip_vsc8541.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/ethernet/phy/phy_microchip_vsc8541.c b/drivers/ethernet/phy/phy_microchip_vsc8541.c index 28d87cc39272..83aa1598e58d 100644 --- a/drivers/ethernet/phy/phy_microchip_vsc8541.c +++ b/drivers/ethernet/phy/phy_microchip_vsc8541.c @@ -230,14 +230,14 @@ static int phy_mc_vsc8541_get_speed(const struct device *dev, struct phy_link_st return ret; } - is_duplex = (aux_status & PHY_REG_PAGE0_EXT_DEV_AUX_DUPLEX) != 0; + is_duplex = (aux_status & PHY_REG_PAGE0_EXT_DEV_AUX_DUPLEX) != 0U; ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_STAT1000_EXT2, &link1000_status); if (ret < 0) { return ret; } - if ((link1000_status & BIT(12))) { + if ((link1000_status & BIT(12)) != 0U) { state->speed = is_duplex ? LINK_FULL_1000BASE : LINK_HALF_1000BASE; return 0; /* no need to check lower speeds */ } @@ -247,7 +247,7 @@ static int phy_mc_vsc8541_get_speed(const struct device *dev, struct phy_link_st return ret; } - if (link100_status & BIT(12)) { + if ((link100_status & BIT(12)) != 0U) { state->speed = is_duplex ? LINK_FULL_100BASE : LINK_HALF_100BASE; return 0; /* no need to check lower speeds */ } @@ -257,7 +257,7 @@ static int phy_mc_vsc8541_get_speed(const struct device *dev, struct phy_link_st return ret; } - if (link10_status & BIT(6)) { + if ((link10_status & BIT(6)) != 0U) { state->speed = is_duplex ? LINK_FULL_10BASE : LINK_HALF_10BASE; } else { state->speed = 0; /* no link */ @@ -269,7 +269,7 @@ static int phy_mc_vsc8541_get_speed(const struct device *dev, struct phy_link_st static int phy_mc_vsc8541_cfg_link(const struct device *dev, enum phy_link_speed adv_speeds, enum phy_cfg_link_flag flags) { - if (flags & PHY_FLAG_AUTO_NEGOTIATION_DISABLED) { + if ((flags & PHY_FLAG_AUTO_NEGOTIATION_DISABLED) != 0U) { LOG_ERR("Disabling auto-negotiation is not supported by this driver"); return -ENOTSUP; } @@ -284,7 +284,6 @@ static int phy_mc_vsc8541_cfg_link(const struct device *dev, enum phy_link_speed static int phy_mc_vsc8541_init(const struct device *dev) { struct mc_vsc8541_data *data = dev->data; - struct mc_vsc8541_config *cfg = dev->config; int ret; data->active_page = -1; @@ -335,7 +334,7 @@ static int phy_mc_vsc8541_get_link(const struct device *dev, struct phy_link_sta hasLink = (reg_sr & MII_BMSR_LINK_STATUS) != 0; - if (reg_cr & MII_BMCR_AUTONEG_ENABLE) { + if ((reg_cr & MII_BMCR_AUTONEG_ENABLE) != 0U) { /* auto negotiation active; update status */ auto_negotiation_finished = (reg_sr & MII_BMSR_AUTONEG_COMPLETE) != 0; } @@ -489,6 +488,7 @@ static int phy_mc_vsc8541_write(const struct device *dev, uint16_t reg_addr, uin static DEVICE_API(ethphy, mc_vsc8541_phy_api) = { .get_link = phy_mc_vsc8541_get_link, + .cfg_link = phy_mc_vsc8541_cfg_link, .link_cb_set = phy_mc_vsc8541_link_cb_set, .read = phy_mc_vsc8541_read, .write = phy_mc_vsc8541_write, From 3ed64ec5865b79070e1d6b89c98b8442e4eef9c9 Mon Sep 17 00:00:00 2001 From: Robert Hancock Date: Thu, 12 Jun 2025 17:11:08 -0600 Subject: [PATCH 4/9] drivers: ethernet: phy: vsc8541: add MDIO enable/disable The driver was not enabling the MDIO bus before trying to access registers. Added enabling and disabling the bus around PHY register accesses. Signed-off-by: Robert Hancock --- drivers/ethernet/phy/phy_microchip_vsc8541.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/ethernet/phy/phy_microchip_vsc8541.c b/drivers/ethernet/phy/phy_microchip_vsc8541.c index 83aa1598e58d..5f39c9f5628c 100644 --- a/drivers/ethernet/phy/phy_microchip_vsc8541.c +++ b/drivers/ethernet/phy/phy_microchip_vsc8541.c @@ -427,6 +427,7 @@ static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint reg_addr &= 0x00ff; k_mutex_lock(&dev_data->mutex, K_FOREVER); + mdio_bus_enable(cfg->mdio_dev); /* select page, given by register upper byte */ if (dev_data->active_page != page) { @@ -441,6 +442,7 @@ static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint ret = mdio_read(cfg->mdio_dev, cfg->addr, reg_addr, (uint16_t *)data); read_end: + mdio_bus_disable(cfg->mdio_dev); k_mutex_unlock(&dev_data->mutex); return ret; @@ -467,6 +469,7 @@ static int phy_mc_vsc8541_write(const struct device *dev, uint16_t reg_addr, uin reg_addr &= 0x00ff; k_mutex_lock(&dev_data->mutex, K_FOREVER); + mdio_bus_enable(cfg->mdio_dev); /* select page, given by register upper byte */ if (dev_data->active_page != page) { @@ -481,6 +484,7 @@ static int phy_mc_vsc8541_write(const struct device *dev, uint16_t reg_addr, uin ret = mdio_write(cfg->mdio_dev, cfg->addr, reg_addr, (uint16_t)data); write_end: + mdio_bus_disable(cfg->mdio_dev); k_mutex_unlock(&dev_data->mutex); return ret; From 346ed0cb8b257f28969b54666d475bb52087c820 Mon Sep 17 00:00:00 2001 From: Robert Hancock Date: Fri, 13 Jun 2025 14:55:12 -0600 Subject: [PATCH 5/9] drivers: ethernet: phy: vsc8541: Use 16-bit values for MDIO access The internal register read/write functions used uint32_t for the values even though the registers are only 16 bits wide, resulting in a bunch of casting. Change the internal functions to use uint16_t and wrap them for the external read/write API which uses uint32_t. Signed-off-by: Robert Hancock --- drivers/ethernet/phy/phy_microchip_vsc8541.c | 58 +++++++++++++------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/drivers/ethernet/phy/phy_microchip_vsc8541.c b/drivers/ethernet/phy/phy_microchip_vsc8541.c index 5f39c9f5628c..5da1e40fb240 100644 --- a/drivers/ethernet/phy/phy_microchip_vsc8541.c +++ b/drivers/ethernet/phy/phy_microchip_vsc8541.c @@ -89,8 +89,8 @@ struct mc_vsc8541_data { uint8_t link_monitor_thread_stack[STACK_SIZE]; }; -static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint32_t *data); -static int phy_mc_vsc8541_write(const struct device *dev, uint16_t reg_addr, uint32_t data); +static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint16_t *data); +static int phy_mc_vsc8541_write(const struct device *dev, uint16_t reg_addr, uint16_t data); static void phy_mc_vsc8541_link_monitor(void *arg1, void *arg2, void *arg3); #if CONFIG_PHY_VERIFY_DEVICE_IDENTIFICATION @@ -102,11 +102,11 @@ static int phy_mc_vsc8541_verify_phy_id(const struct device *dev) uint16_t phy_id_1; uint16_t phy_id_2; - if (phy_mc_vsc8541_read(dev, MII_PHYID1R, (uint32_t *)&phy_id_1) < 0) { + if (phy_mc_vsc8541_read(dev, MII_PHYID1R, &phy_id_1) < 0) { return -EINVAL; } - if (phy_mc_vsc8541_read(dev, MII_PHYID2R, (uint32_t *)&phy_id_2) < 0) { + if (phy_mc_vsc8541_read(dev, MII_PHYID2R, &phy_id_2) < 0) { return -EINVAL; } @@ -133,7 +133,7 @@ static int phy_mc_vsc8541_reset(const struct device *dev) { const struct mc_vsc8541_config *cfg = dev->config; int ret; - uint32_t reg = 0U; + uint16_t reg = 0U; #if DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios) @@ -219,10 +219,10 @@ static int phy_mc_vsc8541_reset(const struct device *dev) static int phy_mc_vsc8541_get_speed(const struct device *dev, struct phy_link_state *state) { int ret; - uint32_t aux_status; - uint32_t link10_status; - uint32_t link100_status; - uint32_t link1000_status; + uint16_t aux_status; + uint16_t link10_status; + uint16_t link100_status; + uint16_t link1000_status; bool is_duplex; ret = phy_mc_vsc8541_read(dev, PHY_REG_PAGE0_EXT_DEV_AUX, &aux_status); @@ -317,8 +317,8 @@ static int phy_mc_vsc8541_init(const struct device *dev) static int phy_mc_vsc8541_get_link(const struct device *dev, struct phy_link_state *state) { int ret; - uint32_t reg_sr; - uint32_t reg_cr; + uint16_t reg_sr; + uint16_t reg_cr; bool hasLink; bool auto_negotiation_finished = true; @@ -412,7 +412,7 @@ void phy_mc_vsc8541_link_monitor(void *arg1, void *arg2, void *arg3) * - to speed up, we store the last used page and only swap page if needed * */ -static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint32_t *data) +static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint16_t *data) { const struct mc_vsc8541_config *cfg = dev->config; struct mc_vsc8541_data *dev_data = dev->data; @@ -421,7 +421,7 @@ static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint *data = 0U; /* decode page */ - uint32_t page = reg_addr >> 8; + uint16_t page = reg_addr >> 8; /* mask out lower byte */ reg_addr &= 0x00ff; @@ -431,7 +431,7 @@ static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint /* select page, given by register upper byte */ if (dev_data->active_page != page) { - ret = mdio_write(cfg->mdio_dev, cfg->addr, PHY_REG_PAGE_SELECTOR, (uint16_t)page); + ret = mdio_write(cfg->mdio_dev, cfg->addr, PHY_REG_PAGE_SELECTOR, page); if (ret < 0) { goto read_end; } @@ -439,7 +439,7 @@ static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint } /* select register, given by register lower byte */ - ret = mdio_read(cfg->mdio_dev, cfg->addr, reg_addr, (uint16_t *)data); + ret = mdio_read(cfg->mdio_dev, cfg->addr, reg_addr, data); read_end: mdio_bus_disable(cfg->mdio_dev); @@ -448,6 +448,15 @@ static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint return ret; } +/** + * @brief Reading of phy register content at given address via mdio interface + * For external API using uint32_t as data type + */ +static int phy_mc_vsc8541_read_ext(const struct device *dev, uint16_t reg_addr, uint32_t *data) +{ + return phy_mc_vsc8541_read(dev, reg_addr, (uint16_t *)data); +} + /** * @brief Writing of new value to phy register at given address via mdio interface * @@ -456,14 +465,14 @@ static int phy_mc_vsc8541_read(const struct device *dev, uint16_t reg_addr, uint * - to speed up, we store the last used page and only swap page if needed * */ -static int phy_mc_vsc8541_write(const struct device *dev, uint16_t reg_addr, uint32_t data) +static int phy_mc_vsc8541_write(const struct device *dev, uint16_t reg_addr, uint16_t data) { const struct mc_vsc8541_config *cfg = dev->config; struct mc_vsc8541_data *dev_data = dev->data; int ret; /* decode page */ - uint32_t page = reg_addr >> 8; + uint16_t page = reg_addr >> 8; /* mask out lower byte */ reg_addr &= 0x00ff; @@ -481,7 +490,7 @@ static int phy_mc_vsc8541_write(const struct device *dev, uint16_t reg_addr, uin } /* write register, given by lower byte */ - ret = mdio_write(cfg->mdio_dev, cfg->addr, reg_addr, (uint16_t)data); + ret = mdio_write(cfg->mdio_dev, cfg->addr, reg_addr, data); write_end: mdio_bus_disable(cfg->mdio_dev); @@ -490,12 +499,21 @@ static int phy_mc_vsc8541_write(const struct device *dev, uint16_t reg_addr, uin return ret; } +/** + * @brief Writing of new value to phy register at given address via mdio interface + * For external API using uint32_t as data type + */ +static int phy_mc_vsc8541_write_ext(const struct device *dev, uint16_t reg_addr, uint32_t data) +{ + return phy_mc_vsc8541_write(dev, reg_addr, (uint16_t)data); +} + static DEVICE_API(ethphy, mc_vsc8541_phy_api) = { .get_link = phy_mc_vsc8541_get_link, .cfg_link = phy_mc_vsc8541_cfg_link, .link_cb_set = phy_mc_vsc8541_link_cb_set, - .read = phy_mc_vsc8541_read, - .write = phy_mc_vsc8541_write, + .read = phy_mc_vsc8541_read_ext, + .write = phy_mc_vsc8541_write_ext, }; #if DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios) From 15d371d5588fd07b20dfda6ceab86019114884a4 Mon Sep 17 00:00:00 2001 From: Robert Hancock Date: Fri, 13 Jun 2025 14:57:07 -0600 Subject: [PATCH 6/9] drivers: ethernet: vsc8541: Fixed inverted reset GPIO For GPIOs driving active-low signals, such as the VSC8541's reset pin, they are supposed to be declared as active low in the device tree, and set to 1 to assert and 0 to clear. Change the driver as such so that it does not leave the PHY stuck in reset when so configured. Also changed all in-tree board DTS files for this PHY to properly declare the reset GPIO as active low. Signed-off-by: Robert Hancock --- boards/sensry/ganymed_bob/ganymed_bob_sy120_gbm.dts | 2 +- boards/sensry/ganymed_bob/ganymed_bob_sy120_gen1.dts | 2 +- boards/sensry/ganymed_sk/ganymed_sk_sy120_gbm.dts | 2 +- boards/sensry/ganymed_sk/ganymed_sk_sy120_gen1.dts | 2 +- drivers/ethernet/phy/phy_microchip_vsc8541.c | 6 +++--- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/boards/sensry/ganymed_bob/ganymed_bob_sy120_gbm.dts b/boards/sensry/ganymed_bob/ganymed_bob_sy120_gbm.dts index 04993ff48f3f..48538044e9aa 100644 --- a/boards/sensry/ganymed_bob/ganymed_bob_sy120_gbm.dts +++ b/boards/sensry/ganymed_bob/ganymed_bob_sy120_gbm.dts @@ -30,7 +30,7 @@ reg = <0x0>; status = "okay"; - reset-gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio0 11 GPIO_ACTIVE_LOW>; microchip,interface-type = "rgmii"; }; diff --git a/boards/sensry/ganymed_bob/ganymed_bob_sy120_gen1.dts b/boards/sensry/ganymed_bob/ganymed_bob_sy120_gen1.dts index 04993ff48f3f..48538044e9aa 100644 --- a/boards/sensry/ganymed_bob/ganymed_bob_sy120_gen1.dts +++ b/boards/sensry/ganymed_bob/ganymed_bob_sy120_gen1.dts @@ -30,7 +30,7 @@ reg = <0x0>; status = "okay"; - reset-gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio0 11 GPIO_ACTIVE_LOW>; microchip,interface-type = "rgmii"; }; diff --git a/boards/sensry/ganymed_sk/ganymed_sk_sy120_gbm.dts b/boards/sensry/ganymed_sk/ganymed_sk_sy120_gbm.dts index b587927127cf..3ef730a681fc 100644 --- a/boards/sensry/ganymed_sk/ganymed_sk_sy120_gbm.dts +++ b/boards/sensry/ganymed_sk/ganymed_sk_sy120_gbm.dts @@ -41,7 +41,7 @@ reg = <0x0b>; status = "okay"; - reset-gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio0 11 GPIO_ACTIVE_LOW>; microchip,interface-type = "rgmii"; }; diff --git a/boards/sensry/ganymed_sk/ganymed_sk_sy120_gen1.dts b/boards/sensry/ganymed_sk/ganymed_sk_sy120_gen1.dts index b587927127cf..3ef730a681fc 100644 --- a/boards/sensry/ganymed_sk/ganymed_sk_sy120_gen1.dts +++ b/boards/sensry/ganymed_sk/ganymed_sk_sy120_gen1.dts @@ -41,7 +41,7 @@ reg = <0x0b>; status = "okay"; - reset-gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio0 11 GPIO_ACTIVE_LOW>; microchip,interface-type = "rgmii"; }; diff --git a/drivers/ethernet/phy/phy_microchip_vsc8541.c b/drivers/ethernet/phy/phy_microchip_vsc8541.c index 5da1e40fb240..6d6cdf0fbfb3 100644 --- a/drivers/ethernet/phy/phy_microchip_vsc8541.c +++ b/drivers/ethernet/phy/phy_microchip_vsc8541.c @@ -143,14 +143,14 @@ static int phy_mc_vsc8541_reset(const struct device *dev) } /* configure the reset pin */ - ret = gpio_pin_configure_dt(&cfg->reset_gpio, GPIO_OUTPUT_ACTIVE); + ret = gpio_pin_configure_dt(&cfg->reset_gpio, GPIO_OUTPUT_INACTIVE); if (ret < 0) { return ret; } for (uint32_t i = 0; i < 2; i++) { /* Start reset */ - ret = gpio_pin_set_dt(&cfg->reset_gpio, 0); + ret = gpio_pin_set_dt(&cfg->reset_gpio, 1); if (ret < 0) { LOG_WRN("failed to set reset gpio"); return -EINVAL; @@ -160,7 +160,7 @@ static int phy_mc_vsc8541_reset(const struct device *dev) k_sleep(K_MSEC(200)); /* Reset over */ - gpio_pin_set_dt(&cfg->reset_gpio, 1); + gpio_pin_set_dt(&cfg->reset_gpio, 0); /* After de-asserting reset, must wait before using the config interface */ k_sleep(K_MSEC(200)); From ad67407cfab402a96f0e618b43dd1c96de09c44c Mon Sep 17 00:00:00 2001 From: Robert Hancock Date: Tue, 17 Jun 2025 11:26:59 -0600 Subject: [PATCH 7/9] doc: migration guide: ethernet: Mention vsc8541 reset changes Mention a change to the reset-gpios handling in the vsc8541 PHY driver. Signed-off-by: Robert Hancock --- doc/releases/migration-guide-4.2.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/releases/migration-guide-4.2.rst b/doc/releases/migration-guide-4.2.rst index e26ef8d01066..d90192bb5f08 100644 --- a/doc/releases/migration-guide-4.2.rst +++ b/doc/releases/migration-guide-4.2.rst @@ -211,6 +211,10 @@ Ethernet * :c:func:`phy_configure_link` got a ``flags`` parameter. Set it to ``0`` to preserve the old behavior (:github:`91354`). +* The :dtcompatible:`microchip,vsc8541` PHY driver now expects the reset-gpios entry to specify + the GPIO_ACTIVE_LOW flag when the reset is being used as active low. Previously the active-low + nature was hard-coded into the driver. (:github:`91726`). + Enhanced Serial Peripheral Interface (eSPI) =========================================== From aa25c6bde75bb90c549738820d66ead01bc19a47 Mon Sep 17 00:00:00 2001 From: Robert Hancock Date: Fri, 13 Jun 2025 15:09:18 -0600 Subject: [PATCH 8/9] drivers: ethernet: phy: vsc8541: Add timeout on SW reset The driver previously could enter an infinite loop if the PHY software reset failed to complete, which could happen due to hardware reset issues or MDIO bus problems. Add a timeout of 1000 iterations so we report an error in this scenario rather than causing a lockup. Signed-off-by: Robert Hancock --- drivers/ethernet/phy/phy_microchip_vsc8541.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/ethernet/phy/phy_microchip_vsc8541.c b/drivers/ethernet/phy/phy_microchip_vsc8541.c index 6d6cdf0fbfb3..afe484af3a19 100644 --- a/drivers/ethernet/phy/phy_microchip_vsc8541.c +++ b/drivers/ethernet/phy/phy_microchip_vsc8541.c @@ -134,6 +134,7 @@ static int phy_mc_vsc8541_reset(const struct device *dev) const struct mc_vsc8541_config *cfg = dev->config; int ret; uint16_t reg = 0U; + int count = 0; #if DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios) @@ -197,8 +198,15 @@ static int phy_mc_vsc8541_reset(const struct device *dev) /* wait for phy finished software reset */ do { - phy_mc_vsc8541_read(dev, MII_BMCR, ®); - } while (reg & MII_BMCR_RESET); + ret = phy_mc_vsc8541_read(dev, MII_BMCR, ®); + if (ret < 0) { + return ret; + } + if (count++ > 1000) { + LOG_ERR("phy reset timed out"); + return -ETIMEDOUT; + } + } while ((reg & MII_BMCR_RESET) != 0U); /* configure the RGMII clk delay */ reg = 0x0; From bdecf2dd7956fa0b9a54efd383478693170be6cd Mon Sep 17 00:00:00 2001 From: Robert Hancock Date: Wed, 18 Jun 2025 13:03:32 -0600 Subject: [PATCH 9/9] drivers: ethernet: phy: vsc8541: allow disabling autonegotiation Add support for disabling autonegotiation to the cfg_link callback, as with the phy_mii driver. Signed-off-by: Robert Hancock --- drivers/ethernet/phy/phy_microchip_vsc8541.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/ethernet/phy/phy_microchip_vsc8541.c b/drivers/ethernet/phy/phy_microchip_vsc8541.c index afe484af3a19..ef7eb3aa52e0 100644 --- a/drivers/ethernet/phy/phy_microchip_vsc8541.c +++ b/drivers/ethernet/phy/phy_microchip_vsc8541.c @@ -277,12 +277,15 @@ static int phy_mc_vsc8541_get_speed(const struct device *dev, struct phy_link_st static int phy_mc_vsc8541_cfg_link(const struct device *dev, enum phy_link_speed adv_speeds, enum phy_cfg_link_flag flags) { + int ret; + if ((flags & PHY_FLAG_AUTO_NEGOTIATION_DISABLED) != 0U) { - LOG_ERR("Disabling auto-negotiation is not supported by this driver"); - return -ENOTSUP; + ret = phy_mii_set_bmcr_reg_autoneg_disabled(dev, adv_speeds); + } else { + ret = phy_mii_cfg_link_autoneg(dev, adv_speeds, true); } - return phy_mii_cfg_link_autoneg(dev, adv_speeds, true); + return ret; } /**