Skip to content

Conversation

TonyHan11
Copy link
Contributor

@TonyHan11 TonyHan11 commented May 28, 2025

This PR includes:

  • add the driver for Microchip KSZ9131 Ethernet PHY
  • add interrupt mode support for KSZ8081

@pdgendt
Copy link
Contributor

pdgendt commented May 28, 2025

@TonyHan11 TonyHan11 force-pushed the phy_ksz9131_ksz8081 branch from 6944d51 to 5055226 Compare May 28, 2025 08:09
@TonyHan11
Copy link
Contributor Author

@pdgendt An entry for ksz9131 added to tests/drivers/build_all/ethernet/app.overlay, thanks.

@maass-hamburg
Copy link
Member

@TonyHan11 pls add PR description and address the things from sonarqube

@TonyHan11
Copy link
Contributor Author

@maass-hamburg updated with removing the unused property, renaming LINK_*BASED_T and fixing if (ret), thank you very much.

@TonyHan11 TonyHan11 force-pushed the phy_ksz9131_ksz8081 branch from 5055226 to cfd8c64 Compare May 29, 2025 01:21
@TonyHan11 TonyHan11 force-pushed the phy_ksz9131_ksz8081 branch 4 times, most recently from 1ac4658 to e7778f7 Compare June 3, 2025 04:41
@TonyHan11
Copy link
Contributor Author

@maass-hamburg
Thank you for your comments. Commits updated with the following changes:

  • using goto instead of do while (0)
  • update if (ret) to if (ret < 0)
  • fix the issues reported by SonarQube Cloud
  • remove comments TODO change this to GPIO interrupt driven

@TonyHan11 TonyHan11 force-pushed the phy_ksz9131_ksz8081 branch 2 times, most recently from ffe8e46 to 94224d7 Compare June 9, 2025 04:51
@TonyHan11
Copy link
Contributor Author

Resolve the CI error by rebasing the commits (without changes).

@TonyHan11 TonyHan11 requested a review from maass-hamburg June 9, 2025 05:30
@TonyHan11 TonyHan11 force-pushed the phy_ksz9131_ksz8081 branch from 94224d7 to 35d32e6 Compare June 10, 2025 09:27
maass-hamburg
maass-hamburg previously approved these changes Jun 10, 2025
reg_val |= PHY_KSZ9131_ICS_LINK_UP_IE_MASK | PHY_KSZ9131_ICS_LINK_DOWN_IE_MASK;

/* Write settings to Interrupt Control/Status register */
ret = ksz9131_write(dev, 27, reg_val);
Copy link
Contributor

@kartben kartben Jun 26, 2025

Choose a reason for hiding this comment

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

Use PHY_KSZ9131_ICS_REG macro instead of magic number

goto done;
}

gpio_init_callback(&data->gpio_callback, phy_mc_ksz8081_interrupt_handler,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
gpio_init_callback(&data->gpio_callback, phy_mc_ksz8081_interrupt_handler,
ret = gpio_init_callback(&data->gpio_callback, phy_mc_ksz8081_interrupt_handler,

@TonyHan11
Copy link
Contributor Author

@TonyHan11 please rebase to current main

rebased, thanks.

@TonyHan11 TonyHan11 force-pushed the phy_ksz9131_ksz8081 branch 2 times, most recently from ef7bf97 to a2640e1 Compare August 4, 2025 09:40
Comment on lines 59 to 61
depends on MDIO
depends on GPIO || (!$(dt_compat_any_has_prop,$(DT_COMPAT_MICROCHIP_KSZ9131),reset-gpios) && \
!$(dt_compat_any_has_prop,$(DT_COMPAT_MICROCHIP_KSZ9131),int-gpios))
Copy link
Member

Choose a reason for hiding this comment

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

apply changes from #94806

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased and updated with the changes applied.

uint16_t anar = 0;
int ret = 0;

(void)config; /* avoid warnings due to config is only used in LOG_DBG() */
Copy link
Member

Choose a reason for hiding this comment

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

use __maybe_unused attribute on config instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

goto done;
}

ret = phy_mchp_ksz9131_autonegotiate(dev);
Copy link
Member

Choose a reason for hiding this comment

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

consider adding the changes from #93118 here too and using phy_mii_cfg_link_autoneg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improve some of the logging.
No changes with auto-negotiate due to no block issue in ksz9131 using interrupt mode.
It seems that the interrupt mode for ksz8081 is not fully supported in the PR.

@TonyHan11
Copy link
Contributor Author

Dropped the commit for adding interrupt mode support for ksz8081 as it has conflicts with the code on mainline.
Will add it in a new PR once it works again.

@TonyHan11
Copy link
Contributor Author

Dropped the commit for adding interrupt mode support for ksz8081 as it has conflicts with the code on mainline. Will add it in a new PR once it works again.

Support for ksz8081 interrupt mode added in #95305

@maass-hamburg maass-hamburg changed the title drivers: ethernet: phy: add driver for ksz9131, update ksz8081 with interrupt support drivers: ethernet: phy: add driver for ksz9131 Sep 2, 2025
@TonyHan11
Copy link
Contributor Author

Updated ksz9131 driver with:

  • use if (USING_INTERRUPT_GPIO) to make code more concise
  • use __maybe_unused for variables in stead of using #if .. #endif
  • split init_int_gpios from phy_mchp_ksz9131_init()

}
}

ret = phy_mchp_ksz9131_get_link(dev, &state);
Copy link
Member

Choose a reason for hiding this comment

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

might be good to apply the changes of 4683dae here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied by adding an additional commit.

Comment on lines 405 to 406
} else {
}
Copy link
Member

Choose a reason for hiding this comment

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

call phy_mchp_ksz9131_speed here, instead of in phy_mchp_ksz9131_get_link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added phy_mchp_ksz9131_speed() to the else branch inside phy_mchp_ksz9131_gigabit().

Comment on lines 340 to 311
ret = phy_mchp_ksz9131_update_anar(dev, adv_speeds);
if (ret < 0) {
goto done;
}

ret = ksz9131_read(dev, MII_1KTCR, &c1kt);
if (ret < 0) {
goto done;
}

if (adv_speeds & LINK_FULL_1000BASE) {
c1kt |= MII_ADVERTISE_1000_FULL;
} else {
c1kt &= ~MII_ADVERTISE_1000_FULL;
}

if (adv_speeds & LINK_HALF_1000BASE) {
c1kt |= MII_ADVERTISE_1000_HALF;
} else {
c1kt &= ~MII_ADVERTISE_1000_HALF;
}

ret = ksz9131_write(dev, MII_1KTCR, c1kt);
if (ret < 0) {
goto done;
}
Copy link
Member

Choose a reason for hiding this comment

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

if you don't want to use phy_mii_cfg_link_autoneg, at least use phy_mii_set_anar_reg and phy_mii_set_c1kt_reg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced this lines with phy_mii_set_anar_reg and phy_mii_set_c1kt_reg, thanks a lot.

Add support for KSZ9131 (Gigabit Ethernet Transceiver with RGMII Support).
As first starter, 100MBit/s mode is tested.
https://www.microchip.com/en-us/product/ksz9131

Signed-off-by: Tony Han <[email protected]>
Add build tests for Microchip KSZ9131.

Signed-off-by: Tony Han <[email protected]>
Enable Link-Up and Link-Down interrupts. On the interrupt handling
the monitor work is scheduled to update the link status and calling
corresponding callback routine.

Signed-off-by: Tony Han <[email protected]>
Read gigabit status from Master Slave Status Register.

Signed-off-by: Tony Han <[email protected]>
@TonyHan11 TonyHan11 force-pushed the phy_ksz9131_ksz8081 branch 3 times, most recently from 477a671 to 2b6b529 Compare September 10, 2025 01:51
Get the link state in the monitor and save it for get_link api
implementation to use.

Signed-off-by: Tony Han <[email protected]>
Copy link

# Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries
# SPDX-License-Identifier: Apache-2.0

description: Microchip KSZ9131 Ethernet PHY device
Copy link
Member

Choose a reason for hiding this comment

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

Hi @TonyHan11 ,

I think you already know:

  • re-order
  • split the content and put in the correct dependency order
  • you need board that uses this phy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants