Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 3, 2025
+83 −23
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
403ba1c
drivers: eth: phy_mii: Don't block system workqueue
kevinior Mar 14, 2025
24c60b1
style: Fix formatting
kevinior Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 83 additions & 23 deletions drivers/ethernet/phy/phy_mii.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,21 @@ struct phy_mii_dev_data {
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

/* How often to poll auto-negotiation status while waiting for it to complete */
#define MII_AUTONEG_POLL_INTERVAL_MS 100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kconfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't configurable before, so making it configurable now seems like a separate issue from the one I'm trying to fix here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this file is refactored later, I would like to add, that the timeout could now be implemented using the timepoint_expired-function.
Not needed for this PR though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't configurable before, so making it configurable now seems like a separate issue from the one I'm trying to fix here.

I thought you are adding this in this PR, I am suggesting it be added in a configurable way rather than hardcoded

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't configurable before, so making it configurable now seems like a separate issue from the one I'm trying to fix here.

I thought you are adding this in this PR, I am suggesting it be added in a configurable way rather than hardcoded

No, the polling interval existed before and was not configurable.


static int phy_mii_get_link_state(const struct device *dev,
struct phy_link_state *state);

Expand Down Expand Up @@ -155,13 +160,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;
Expand All @@ -188,11 +188,6 @@ static int update_link_state(const struct device *dev)
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;
}

/* Configure and start auto-negotiation process */
if (phy_mii_reg_read(dev, MII_BMCR, &bmcr_reg) < 0) {
return -EIO;
Expand All @@ -205,15 +200,21 @@ static int update_link_state(const struct device *dev)
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;

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;

/* On some PHY chips, the BMSR bits are latched, so the first read may
* show incorrect status. A second read ensures correct values.
Expand All @@ -226,11 +227,23 @@ static int update_link_state(const struct device *dev)
if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
return -EIO;
}
} while (!(bmsr_reg & MII_BMSR_AUTONEG_COMPLETE));

if (!(bmsr_reg & MII_BMSR_AUTONEG_COMPLETE)) {
if (data->autoneg_timeout-- == 0U) {
LOG_DBG("PHY (%d) auto-negotiate timedout", cfg->phy_addr);
return -ETIMEDOUT;
}
return -EINPROGRESS;
}

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) {
return -EIO;
Expand Down Expand Up @@ -293,7 +306,12 @@ 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);

Expand All @@ -304,9 +322,49 @@ 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 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,
Expand Down Expand Up @@ -499,6 +557,8 @@ static int phy_mii_initialize(const struct device *dev)

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);
}
Expand Down