drivers: eth: phy_mii: Don't block system workqueue

Looping while waiting for auto-negotiation to complete can block the
system workqueue for several seconds.

Signed-off-by: Kevin ORourke <[email protected]>

Author: kevinior

drivers/ethernet/phy/phy_mii.c

@@ -23,29 +23,32 @@ struct phy_mii_dev_config {
 	bool no_reset;
 	bool fixed;
 	int fixed_speed;
-	const struct device * const mdio;
+	const struct device *const mdio;
 };
 
 struct phy_mii_dev_data {
 	const struct device *dev;
 	phy_callback_t cb;
 	void *cb_data;
 	struct k_work_delayable monitor_work;
+	struct k_work_delayable autoneg_work;
 	struct phy_link_state state;
 	struct k_sem sem;
 	bool gigabit_supported;
+	uint32_t autoneg_timeout;
 };
 
 /* Offset to align capabilities bits of 1000BASE-T Control and Status regs */
 #define MII_1KSTSR_OFFSET 2
 
 #define MII_INVALID_PHY_ID UINT32_MAX
 
-static int phy_mii_get_link_state(const struct device *dev,
-				  struct phy_link_state *state);
+/* How often to poll auto-negotiation status while waiting for it to complete */
+#define MII_AUTONEG_POLL_INTERVAL_MS 100
 
-static inline int phy_mii_reg_read(const struct device *dev, uint16_t reg_addr,
-			   uint16_t *value)
+static int phy_mii_get_link_state(const struct device *dev, struct phy_link_state *state);
+
+static inline int phy_mii_reg_read(const struct device *dev, uint16_t reg_addr, uint16_t *value)
 {
 	const struct phy_mii_dev_config *const cfg = dev->config;
 
@@ -61,8 +64,7 @@ static inline int phy_mii_reg_read(const struct device *dev, uint16_t reg_addr,
 	return mdio_read(cfg->mdio, cfg->phy_addr, reg_addr, value);
 }
 
-static inline int phy_mii_reg_write(const struct device *dev, uint16_t reg_addr,
-			    uint16_t value)
+static inline int phy_mii_reg_write(const struct device *dev, uint16_t reg_addr, uint16_t value)
 {
 	const struct phy_mii_dev_config *const cfg = dev->config;
 
@@ -92,8 +94,7 @@ static bool is_gigabit_supported(const struct device *dev)
 			return -EIO;
 		}
 
-		if (estat_reg & (MII_ESTAT_1000BASE_T_HALF
-				 | MII_ESTAT_1000BASE_T_FULL)) {
+		if (estat_reg & (MII_ESTAT_1000BASE_T_HALF | MII_ESTAT_1000BASE_T_FULL)) {
 			return true;
 		}
 	}
@@ -155,13 +156,8 @@ static int update_link_state(const struct device *dev)
 	struct phy_mii_dev_data *const data = dev->data;
 	bool link_up;
 
-	uint16_t anar_reg = 0;
 	uint16_t bmcr_reg = 0;
 	uint16_t bmsr_reg = 0;
-	uint16_t anlpar_reg = 0;
-	uint16_t c1kt_reg = 0;
-	uint16_t s1kt_reg = 0;
-	uint32_t timeout = CONFIG_PHY_AUTONEG_TIMEOUT_MS / 100;
 
 	if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
 		return -EIO;
@@ -185,51 +181,62 @@ static int update_link_state(const struct device *dev)
 	/**
 	 * Perform auto-negotiation sequence.
 	 */
-	LOG_DBG("PHY (%d) Starting MII PHY auto-negotiate sequence",
-		cfg->phy_addr);
-
-	/* Read PHY default advertising parameters */
-	if (phy_mii_reg_read(dev, MII_ANAR, &anar_reg) < 0) {
-		return -EIO;
-	}
+	LOG_DBG("PHY (%d) Starting MII PHY auto-negotiate sequence", cfg->phy_addr);
 
 	/* Configure and start auto-negotiation process */
 	if (phy_mii_reg_read(dev, MII_BMCR, &bmcr_reg) < 0) {
 		return -EIO;
 	}
 
 	bmcr_reg |= MII_BMCR_AUTONEG_ENABLE | MII_BMCR_AUTONEG_RESTART;
-	bmcr_reg &= ~MII_BMCR_ISOLATE;  /* Don't isolate the PHY */
+	bmcr_reg &= ~MII_BMCR_ISOLATE; /* Don't isolate the PHY */
 
 	if (phy_mii_reg_write(dev, MII_BMCR, bmcr_reg) < 0) {
 		return -EIO;
 	}
 
-	/* Wait for the auto-negotiation process to complete */
-	do {
-		if (timeout-- == 0U) {
-			LOG_DBG("PHY (%d) auto-negotiate timedout",
-				cfg->phy_addr);
-			return -ETIMEDOUT;
-		}
+	/* We have to wait for the auto-negotiation process to complete */
+	data->autoneg_timeout = CONFIG_PHY_AUTONEG_TIMEOUT_MS / MII_AUTONEG_POLL_INTERVAL_MS;
+	return -EINPROGRESS;
+}
 
-		k_sleep(K_MSEC(100));
+static int check_autonegotiation_completion(const struct device *dev)
+{
+	const struct phy_mii_dev_config *const cfg = dev->config;
+	struct phy_mii_dev_data *const data = dev->data;
 
-		/* On some PHY chips, the BMSR bits are latched, so the first read may
-		 * show incorrect status. A second read ensures correct values.
-		 */
-		if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
-			return -EIO;
-		}
+	uint16_t anar_reg = 0;
+	uint16_t bmsr_reg = 0;
+	uint16_t anlpar_reg = 0;
+	uint16_t c1kt_reg = 0;
+	uint16_t s1kt_reg = 0;
 
-		/* Second read, clears the latched bits and gives the correct status */
-		if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
-			return -EIO;
+	/* On some PHY chips, the BMSR bits are latched, so the first read may
+	 * show incorrect status. A second read ensures correct values.
+	 */
+	if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
+		return -EIO;
+	}
+
+	/* Second read, clears the latched bits and gives the correct status */
+	if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
+		return -EIO;
+	}
+
+	if (!(bmsr_reg & MII_BMSR_AUTONEG_COMPLETE)) {
+		if (data->autoneg_timeout-- == 0U) {
+			LOG_DBG("PHY (%d) auto-negotiate timedout", cfg->phy_addr);
+			return -ETIMEDOUT;
 		}
-	} while (!(bmsr_reg & MII_BMSR_AUTONEG_COMPLETE));
+		return -EINPROGRESS;
+	}
 
-	LOG_DBG("PHY (%d) auto-negotiate sequence completed",
-		cfg->phy_addr);
+	LOG_DBG("PHY (%d) auto-negotiate sequence completed", cfg->phy_addr);
+
+	/* Read PHY default advertising parameters */
+	if (phy_mii_reg_read(dev, MII_ANAR, &anar_reg) < 0) {
+		return -EIO;
+	}
 
 	/** Read peer device capability */
 	if (phy_mii_reg_read(dev, MII_ANLPAR, &anlpar_reg) < 0) {
@@ -246,11 +253,9 @@ static int update_link_state(const struct device *dev)
 		s1kt_reg = (uint16_t)(s1kt_reg >> MII_1KSTSR_OFFSET);
 	}
 
-	if (data->gigabit_supported &&
-			((c1kt_reg & s1kt_reg) & MII_ADVERTISE_1000_FULL)) {
+	if (data->gigabit_supported && ((c1kt_reg & s1kt_reg) & MII_ADVERTISE_1000_FULL)) {
 		data->state.speed = LINK_FULL_1000BASE_T;
-	} else if (data->gigabit_supported &&
-			((c1kt_reg & s1kt_reg) & MII_ADVERTISE_1000_HALF)) {
+	} else if (data->gigabit_supported && ((c1kt_reg & s1kt_reg) & MII_ADVERTISE_1000_HALF)) {
 		data->state.speed = LINK_HALF_1000BASE_T;
 	} else if ((anar_reg & anlpar_reg) & MII_ADVERTISE_100_FULL) {
 		data->state.speed = LINK_FULL_100BASE_T;
@@ -262,10 +267,10 @@ static int update_link_state(const struct device *dev)
 		data->state.speed = LINK_HALF_10BASE_T;
 	}
 
-	LOG_INF("PHY (%d) Link speed %s Mb, %s duplex",
-		cfg->phy_addr,
-		PHY_LINK_IS_SPEED_1000M(data->state.speed) ? "1000" :
-		(PHY_LINK_IS_SPEED_100M(data->state.speed) ? "100" : "10"),
+	LOG_INF("PHY (%d) Link speed %s Mb, %s duplex", cfg->phy_addr,
+		PHY_LINK_IS_SPEED_1000M(data->state.speed)
+			? "1000"
+			: (PHY_LINK_IS_SPEED_100M(data->state.speed) ? "100" : "10"),
 		PHY_LINK_IS_FULL_DUPLEX(data->state.speed) ? "full" : "half");
 
 	return 0;
@@ -293,7 +298,11 @@ static void monitor_work_handler(struct k_work *work)
 	const struct device *dev = data->dev;
 	int rc;
 
-	k_sem_take(&data->sem, K_FOREVER);
+	if (k_sem_take(&data->sem, K_NO_WAIT) != 0) {
+		/* Try again soon */
+		k_work_reschedule(&data->monitor_work, K_MSEC(MII_AUTONEG_POLL_INTERVAL_MS));
+		return;
+	}
 
 	rc = update_link_state(dev);
 
@@ -304,25 +313,58 @@ static void monitor_work_handler(struct k_work *work)
 		invoke_link_cb(dev);
 	}
 
-	/* Submit delayed work */
-	k_work_reschedule(&data->monitor_work,
-			  K_MSEC(CONFIG_PHY_MONITOR_PERIOD));
+	if (rc == -EINPROGRESS) {
+		/* Check for autonegotiation completion */
+		k_work_reschedule(&data->autoneg_work, K_MSEC(MII_AUTONEG_POLL_INTERVAL_MS));
+	} else {
+		/* Submit delayed work */
+		k_work_reschedule(&data->monitor_work, K_MSEC(CONFIG_PHY_MONITOR_PERIOD));
+	}
 }
 
-static int phy_mii_read(const struct device *dev, uint16_t reg_addr,
-			uint32_t *data)
+static void autoneg_work_handler(struct k_work *work)
+{
+	struct k_work_delayable *dwork = k_work_delayable_from_work(work);
+	struct phy_mii_dev_data *const data =
+		CONTAINER_OF(dwork, struct phy_mii_dev_data, autoneg_work);
+	const struct device *dev = data->dev;
+	int rc;
+
+	if (k_sem_take(&data->sem, K_NO_WAIT) != 0) {
+		/* Try again soon */
+		k_work_reschedule(&data->autoneg_work, K_MSEC(MII_AUTONEG_POLL_INTERVAL_MS));
+		return;
+	}
+
+	rc = check_autonegotiation_completion(dev);
+
+	k_sem_give(&data->sem);
+
+	/* If link state has changed and a callback is set, invoke callback */
+	if (rc == 0) {
+		invoke_link_cb(dev);
+	}
+
+	if (rc == -EINPROGRESS) {
+		/* Check again soon */
+		k_work_reschedule(&data->autoneg_work, K_MSEC(MII_AUTONEG_POLL_INTERVAL_MS));
+	} else {
+		/* Schedule the next monitoring call */
+		k_work_reschedule(&data->monitor_work, K_MSEC(CONFIG_PHY_MONITOR_PERIOD));
+	}
+}
+
+static int phy_mii_read(const struct device *dev, uint16_t reg_addr, uint32_t *data)
 {
 	return phy_mii_reg_read(dev, reg_addr, (uint16_t *)data);
 }
 
-static int phy_mii_write(const struct device *dev, uint16_t reg_addr,
-			 uint32_t data)
+static int phy_mii_write(const struct device *dev, uint16_t reg_addr, uint32_t data)
 {
 	return phy_mii_reg_write(dev, reg_addr, (uint16_t)data);
 }
 
-static int phy_mii_cfg_link(const struct device *dev,
-			    enum phy_link_speed adv_speeds)
+static int phy_mii_cfg_link(const struct device *dev, enum phy_link_speed adv_speeds)
 {
 	struct phy_mii_dev_data *const data = dev->data;
 	const struct phy_mii_dev_config *const cfg = dev->config;
@@ -408,8 +450,7 @@ static int phy_mii_cfg_link(const struct device *dev,
 	return 0;
 }
 
-static int phy_mii_get_link_state(const struct device *dev,
-				  struct phy_link_state *state)
+static int phy_mii_get_link_state(const struct device *dev, struct phy_link_state *state)
 {
 	struct phy_mii_dev_data *const data = dev->data;
 
@@ -422,8 +463,7 @@ static int phy_mii_get_link_state(const struct device *dev,
 	return 0;
 }
 
-static int phy_mii_link_cb_set(const struct device *dev, phy_callback_t cb,
-			       void *user_data)
+static int phy_mii_link_cb_set(const struct device *dev, phy_callback_t cb, void *user_data)
 {
 	struct phy_mii_dev_data *const data = dev->data;
 
@@ -457,12 +497,8 @@ static int phy_mii_initialize(const struct device *dev)
 	 */
 	if (cfg->fixed) {
 		const static int speed_to_phy_link_speed[] = {
-			LINK_HALF_10BASE_T,
-			LINK_FULL_10BASE_T,
-			LINK_HALF_100BASE_T,
-			LINK_FULL_100BASE_T,
-			LINK_HALF_1000BASE_T,
-			LINK_FULL_1000BASE_T,
+			LINK_HALF_10BASE_T,  LINK_FULL_10BASE_T,   LINK_HALF_100BASE_T,
+			LINK_FULL_100BASE_T, LINK_HALF_1000BASE_T, LINK_FULL_1000BASE_T,
 		};
 
 		data->state.speed = speed_to_phy_link_speed[cfg->fixed_speed];
@@ -478,8 +514,7 @@ static int phy_mii_initialize(const struct device *dev)
 
 		if (get_id(dev, &phy_id) == 0) {
 			if (phy_id == MII_INVALID_PHY_ID) {
-				LOG_ERR("No PHY found at address %d",
-					cfg->phy_addr);
+				LOG_ERR("No PHY found at address %d", cfg->phy_addr);
 
 				return -EINVAL;
 			}
@@ -490,23 +525,20 @@ static int phy_mii_initialize(const struct device *dev)
 		data->gigabit_supported = is_gigabit_supported(dev);
 
 		/* Advertise all speeds */
-		phy_mii_cfg_link(dev, LINK_HALF_10BASE_T |
-				      LINK_FULL_10BASE_T |
-				      LINK_HALF_100BASE_T |
-				      LINK_FULL_100BASE_T |
-				      LINK_HALF_1000BASE_T |
-				      LINK_FULL_1000BASE_T);
+		phy_mii_cfg_link(dev, LINK_HALF_10BASE_T | LINK_FULL_10BASE_T |
+					      LINK_HALF_100BASE_T | LINK_FULL_100BASE_T |
+					      LINK_HALF_1000BASE_T | LINK_FULL_1000BASE_T);
 
-		k_work_init_delayable(&data->monitor_work,
-					monitor_work_handler);
+		k_work_init_delayable(&data->monitor_work, monitor_work_handler);
+		k_work_init_delayable(&data->autoneg_work, autoneg_work_handler);
 
 		monitor_work_handler(&data->monitor_work.work);
 	}
 
 	return 0;
 }
 
-#define IS_FIXED_LINK(n)	DT_INST_NODE_HAS_PROP(n, fixed_link)
+#define IS_FIXED_LINK(n) DT_INST_NODE_HAS_PROP(n, fixed_link)
 
 static DEVICE_API(ethphy, phy_mii_driver_api) = {
 	.get_link = phy_mii_get_link_state,
@@ -516,25 +548,19 @@ static DEVICE_API(ethphy, phy_mii_driver_api) = {
 	.write = phy_mii_write,
 };
 
-#define PHY_MII_CONFIG(n)						 \
-static const struct phy_mii_dev_config phy_mii_dev_config_##n = {	 \
-	.phy_addr = DT_INST_REG_ADDR(n),				 \
-	.no_reset = DT_INST_PROP(n, no_reset),				 \
-	.fixed = IS_FIXED_LINK(n),					 \
-	.fixed_speed = DT_INST_ENUM_IDX_OR(n, fixed_link, 0),		 \
-	.mdio = UTIL_AND(UTIL_NOT(IS_FIXED_LINK(n)),			 \
-			 DEVICE_DT_GET(DT_INST_BUS(n)))			 \
-};
-
-#define PHY_MII_DEVICE(n)						\
-	PHY_MII_CONFIG(n);						\
-	static struct phy_mii_dev_data phy_mii_dev_data_##n;		\
-	DEVICE_DT_INST_DEFINE(n,					\
-			      &phy_mii_initialize,			\
-			      NULL,					\
-			      &phy_mii_dev_data_##n,			\
-			      &phy_mii_dev_config_##n, POST_KERNEL,	\
-			      CONFIG_PHY_INIT_PRIORITY,			\
+#define PHY_MII_CONFIG(n)                                                                          \
+	static const struct phy_mii_dev_config phy_mii_dev_config_##n = {                          \
+		.phy_addr = DT_INST_REG_ADDR(n),                                                   \
+		.no_reset = DT_INST_PROP(n, no_reset),                                             \
+		.fixed = IS_FIXED_LINK(n),                                                         \
+		.fixed_speed = DT_INST_ENUM_IDX_OR(n, fixed_link, 0),                              \
+		.mdio = UTIL_AND(UTIL_NOT(IS_FIXED_LINK(n)), DEVICE_DT_GET(DT_INST_BUS(n)))};
+
+#define PHY_MII_DEVICE(n)                                                                          \
+	PHY_MII_CONFIG(n);                                                                         \
+	static struct phy_mii_dev_data phy_mii_dev_data_##n;                                       \
+	DEVICE_DT_INST_DEFINE(n, &phy_mii_initialize, NULL, &phy_mii_dev_data_##n,                 \
+			      &phy_mii_dev_config_##n, POST_KERNEL, CONFIG_PHY_INIT_PRIORITY,      \
 			      &phy_mii_driver_api);
 
 DT_INST_FOREACH_STATUS_OKAY(PHY_MII_DEVICE)