Skip to content

drivers: ethernet: phy: add driver for ksz9131, update ksz8081 with interrupt support #90711

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.

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

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
Collaborator

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
Collaborator

@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
Collaborator

@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

Comment on lines +387 to +480
mutual_capabilities = anar & anlpar;
if (mutual_capabilities & MII_ADVERTISE_100_FULL) {
state->speed = LINK_FULL_100BASE;
} else if (mutual_capabilities & MII_ADVERTISE_100_HALF) {
state->speed = LINK_HALF_100BASE;
} else if (mutual_capabilities & MII_ADVERTISE_10_FULL) {
state->speed = LINK_FULL_10BASE;
} else if (mutual_capabilities & MII_ADVERTISE_10_HALF) {
state->speed = LINK_HALF_10BASE;
} else {
ret = -EIO;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

no gigabit?

goto done;
}

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

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

@kartben Thank you for your comments.

updated with the following changes:

  • replace the magic number
  • add gigabit support
  • remove extra if (ret < 0) { after gpio_init_callback() due to it's a void routine

Comment on lines 380 to 404
/* Read AUTO-NEGOTIATION MASTER SLAVE STATUS REGISTER */
ret = ksz9131_read(dev, PHY_KSZ9131_MSS_REG, &mssr);
if (ret < 0) {
goto done;
} else if (mssr & PHY_KSZ9131_MSS_ADVERTISE_1000_FULL) {
state->speed = LINK_FULL_1000BASE;
goto done;
} else if (mssr & PHY_KSZ9131_MSS_ADVERTISE_1000_HALF) {
state->speed = LINK_HALF_1000BASE;
goto done;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. these are generic registers, use the deines from mii.h for that
  2. you have to read the status and the controll register to get the mutal_capabilities, like with the 100/10 registers later. take a look at phy_mii.c how to do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comments.

According to the datasheet, 1000Mbps is related with the following bits:

  • 5.2.1 BASIC CONTROL REGISTER (index MII_BMCR) bit 13 and bit 6
    if (bmsr & MII_BMCR_SPEED_MASK == MII_BMCR_SPEED_1000)
  • 5.2.10 AUTO-NEGOTIATION MASTER SLAVE CONTROL REGISTER (index MII_1KTCR) bit 9 and bit 8
	9 1000BASE-T Full Duplex
		0 = advertise PHY is not 1000BASE-T full duplex capable
		1 = advertise PHY is 1000BASE-T full duplex capable
	8 1000BASE-T Half Duplex
		0 = advertise PHY is not 1000BASE-T half duplex capable
		1 = advertise PHY is 1000BASE-T half duplex capable
  • 5.2.11 AUTO-NEGOTIATION MASTER SLAVE STATUS REGISTER (index MII_1KSTSR) bit 11 and bit 10
	11 Link Partner Advertised 1000BASE-T Full Duplex Capability
		0 = Link Partner is not capable of 1000BASE-T full duplex
		1 = Link Partner is capable of 1000BASE-T full duplex
	10 Link Partner Advertised 1000BASE-T Half Duplex Capability
		0 = Link Partner is not capable of 1000BASE-T half duplex
		1 = Link Partner is capable of 1000BASE-T half duplex

If it is acceptable with the following? Thanks.

	/* Read AUTO-NEGOTIATION MASTER SLAVE CONTROL REGISTER */
	ret = ksz9131_read(dev, MII_1KTCR, &mscr);
	if (ret < 0) {
		goto done;
	}

	/* Read AUTO-NEGOTIATION MASTER SLAVE STATUS REGISTER */
	ret = ksz9131_read(dev, MII_1KSTSR, &mssr);
	if (ret < 0) {
		goto done;
	}

	if (mscr & MII_ADVERTISE_1000_FULL) {
		if (mssr & PHY_KSZ9131_MSS_ADVERTISE_1000_FULL) {
			state->speed = LINK_FULL_1000BASE;
			goto done;
	}else if (mscr & MII_ADVERTISE_1000_HALF) {
		if (mssr & PHY_KSZ9131_MSS_ADVERTISE_1000_HALF) {
			state->speed = LINK_HALF_1000BASE;
			goto done;
	}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer, if you would reuse the code of the phy_mii.c

	if (data->gigabit_supported) {
		if (phy_mii_reg_read(dev, MII_1KTCR, &c1kt_reg) < 0) {
			return -EIO;
		}
		if (phy_mii_reg_read(dev, MII_1KSTSR, &s1kt_reg) < 0) {
			return -EIO;
		}
		s1kt_reg = (uint16_t)(s1kt_reg >> MII_1KSTSR_OFFSET);
	}

	if (data->gigabit_supported &&
			((c1kt_reg & s1kt_reg & MII_ADVERTISE_1000_FULL) != 0U)) {
		data->state.speed = LINK_FULL_1000BASE;
	} else if (data->gigabit_supported &&
			((c1kt_reg & s1kt_reg & MII_ADVERTISE_1000_HALF) != 0U)) {
		data->state.speed = LINK_HALF_1000BASE;

and just shift the MII_1KSTSR register by 2.

Btw good way to only read the 100/10 Mbit/s regs, when gigabit doesn't match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks.

Comment on lines 44 to 45
#define PHY_KSZ9131_MSS_REG 10
#define PHY_KSZ9131_MSS_ADVERTISE_1000_FULL BIT(11)
#define PHY_KSZ9131_MSS_ADVERTISE_1000_HALF BIT(10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

unneded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No suitable macros found in mii.h for these 2 bits. No sure if it's special for KSZ9131.

@TonyHan11 TonyHan11 force-pushed the phy_ksz9131_ksz8081 branch 4 times, most recently from 8892b25 to 6f9aced Compare June 30, 2025 04:53
@TonyHan11 TonyHan11 requested a review from maass-hamburg June 30, 2025 05:33
@kartben kartben added this to the v4.3.0 milestone Jul 1, 2025
@TonyHan11 TonyHan11 force-pushed the phy_ksz9131_ksz8081 branch 2 times, most recently from 27c6e59 to 5a9a2c6 Compare July 2, 2025 03:33
TonyHan11 added 5 commits July 2, 2025 11:41
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]>
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]>
Copy link

sonarqubecloud bot commented Jul 2, 2025

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.

4 participants