Skip to content

drivers: ethernet: drop _T from ethernet speed types #87194

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 4 commits into
base: main
Choose a base branch
from

Conversation

maass-hamburg
Copy link
Collaborator

@maass-hamburg maass-hamburg commented Mar 17, 2025

We shouldn't have speed options for ethernet drivers, which end with a _T, that imply that ethernet is only supported via a twisted pair cable.

pdgendt
pdgendt previously approved these changes Mar 17, 2025
jukkar
jukkar previously approved these changes Mar 17, 2025
decsny
decsny previously approved these changes Mar 17, 2025
@maass-hamburg maass-hamburg added the DNM This PR should not be merged (Do Not Merge) label Mar 18, 2025
@maass-hamburg
Copy link
Collaborator Author

for 10 Mbits and 100 Mbits phys are not able to distinguish between BASE-T and BASE-X, renaming them would actually be the better option

@pdgendt
Copy link
Collaborator

pdgendt commented Mar 18, 2025

I wonder, should we add backwards compatible defines to denote deprecation and give downstream users time to adopt?

@maass-hamburg
Copy link
Collaborator Author

I wonder, should we add backwards compatible defines to denote deprecation and give downstream users time to adopt?

Due to it being a pretty simply and trivial change, I would prefer to not do that. Also the ethernet API is still considered unstable according to https://docs.zephyrproject.org/latest/develop/api/overview.html#api-overview, so it is not required.

decsny
decsny previously requested changes Mar 18, 2025
Copy link
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

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

I really do not see the point of this. And how then would someone support something like the Marvel Alaska 88e151x PHY which can support both Copper and Fiber and be switched between them programmatically.

https://www.google.com/url?sa=t&source=web&rct=j&opi=89978449&url=https://www.marvell.com/content/dam/marvell/en/public-collateral/phys-transceivers/marvell-phys-transceivers-alaska-88e151x-datasheet.pdf&ved=2ahUKEwjix87f65OMAxXBI0QIHVbaIv4QFnoECAkQAQ&usg=AOvVaw1Kq4IQlJv0V1DiHLDe4d5g

see Page 117, Table 130, General Control Register 1, bits 2:0 for mode selection

@maass-hamburg
Copy link
Collaborator Author

I really do not see the point of this. And how then would someone support something like the Marvel Alaska 88e151x PHY which can support both Copper and Fiber and be switched between them programmatically.

https://www.google.com/url?sa=t&source=web&rct=j&opi=89978449&url=https://www.marvell.com/content/dam/marvell/en/public-collateral/phys-transceivers/marvell-phys-transceivers-alaska-88e151x-datasheet.pdf&ved=2ahUKEwjix87f65OMAxXBI0QIHVbaIv4QFnoECAkQAQ&usg=AOvVaw1Kq4IQlJv0V1DiHLDe4d5g

see Page 117, Table 130, General Control Register 1, bits 2:0 for mode selection

I think we have to differentiate between the communication between the mac/ethernet driver and the phy and other communication. API calls like phy_configure_link(), phy_get_link_state and also the callback are mainly used between the ethernet driver and the phy driver. Between the there we have the MII, the media independent interface, and because of that the ethernet driver doesn't needs to know that media (like copper or fiber) is used. and I like to keep the communication between the drivers the same.

But what I see what we might also need is a api to set the media or used standards of the phy.

@decsny
Copy link
Member

decsny commented Mar 18, 2025

I really do not see the point of this. And how then would someone support something like the Marvel Alaska 88e151x PHY which can support both Copper and Fiber and be switched between them programmatically.
https://www.google.com/url?sa=t&source=web&rct=j&opi=89978449&url=https://www.marvell.com/content/dam/marvell/en/public-collateral/phys-transceivers/marvell-phys-transceivers-alaska-88e151x-datasheet.pdf&ved=2ahUKEwjix87f65OMAxXBI0QIHVbaIv4QFnoECAkQAQ&usg=AOvVaw1Kq4IQlJv0V1DiHLDe4d5g
see Page 117, Table 130, General Control Register 1, bits 2:0 for mode selection

I think we have to differentiate between the communication between the mac/ethernet driver and the phy and other communication. API calls like phy_configure_link(), phy_get_link_state and also the callback are mainly used between the ethernet driver and the phy driver. Between the there we have the MII, the media independent interface, and because of that the ethernet driver doesn't needs to know that media (like copper or fiber) is used. and I like to keep the communication between the drivers the same.

But what I see what we might also need is a api to set the media or used standards of the phy.

Maybe it can be a new field in the phy_link_state struct .

@maass-hamburg
Copy link
Collaborator Author

I really do not see the point of this. And how then would someone support something like the Marvel Alaska 88e151x PHY which can support both Copper and Fiber and be switched between them programmatically.
https://www.google.com/url?sa=t&source=web&rct=j&opi=89978449&url=https://www.marvell.com/content/dam/marvell/en/public-collateral/phys-transceivers/marvell-phys-transceivers-alaska-88e151x-datasheet.pdf&ved=2ahUKEwjix87f65OMAxXBI0QIHVbaIv4QFnoECAkQAQ&usg=AOvVaw1Kq4IQlJv0V1DiHLDe4d5g
see Page 117, Table 130, General Control Register 1, bits 2:0 for mode selection

I think we have to differentiate between the communication between the mac/ethernet driver and the phy and other communication. API calls like phy_configure_link(), phy_get_link_state and also the callback are mainly used between the ethernet driver and the phy driver. Between the there we have the MII, the media independent interface, and because of that the ethernet driver doesn't needs to know that media (like copper or fiber) is used. and I like to keep the communication between the drivers the same.
But what I see what we might also need is a api to set the media or used standards of the phy.

Maybe it can be a new field in the phy_link_state struct .

That could be a solution. But that should be part of another PR. I'm a bit more in favor in having separate set and get config api functions for that, so that the existing phy_configure_link() and phy_get_link_state() can stay media independent for the communication with the mac driver.

@decsny
Copy link
Member

decsny commented Mar 19, 2025

I really do not see the point of this. And how then would someone support something like the Marvel Alaska 88e151x PHY which can support both Copper and Fiber and be switched between them programmatically.
https://www.google.com/url?sa=t&source=web&rct=j&opi=89978449&url=https://www.marvell.com/content/dam/marvell/en/public-collateral/phys-transceivers/marvell-phys-transceivers-alaska-88e151x-datasheet.pdf&ved=2ahUKEwjix87f65OMAxXBI0QIHVbaIv4QFnoECAkQAQ&usg=AOvVaw1Kq4IQlJv0V1DiHLDe4d5g
see Page 117, Table 130, General Control Register 1, bits 2:0 for mode selection

I think we have to differentiate between the communication between the mac/ethernet driver and the phy and other communication. API calls like phy_configure_link(), phy_get_link_state and also the callback are mainly used between the ethernet driver and the phy driver. Between the there we have the MII, the media independent interface, and because of that the ethernet driver doesn't needs to know that media (like copper or fiber) is used. and I like to keep the communication between the drivers the same.
But what I see what we might also need is a api to set the media or used standards of the phy.

Maybe it can be a new field in the phy_link_state struct .

That could be a solution. But that should be part of another PR. I'm a bit more in favor in having separate set and get config api functions for that, so that the existing phy_configure_link() and phy_get_link_state() can stay media independent for the communication with the mac driver.

I am envisioning an enum member field with options of like twisted pair, fiber, or any/unspecified

@maass-hamburg maass-hamburg force-pushed the phy_add_basex branch 2 times, most recently from 624ea67 to 2533459 Compare March 20, 2025 09:23
@maass-hamburg
Copy link
Collaborator Author

I really do not see the point of this. And how then would someone support something like the Marvel Alaska 88e151x PHY which can support both Copper and Fiber and be switched between them programmatically.
https://www.google.com/url?sa=t&source=web&rct=j&opi=89978449&url=https://www.marvell.com/content/dam/marvell/en/public-collateral/phys-transceivers/marvell-phys-transceivers-alaska-88e151x-datasheet.pdf&ved=2ahUKEwjix87f65OMAxXBI0QIHVbaIv4QFnoECAkQAQ&usg=AOvVaw1Kq4IQlJv0V1DiHLDe4d5g
see Page 117, Table 130, General Control Register 1, bits 2:0 for mode selection

I think we have to differentiate between the communication between the mac/ethernet driver and the phy and other communication. API calls like phy_configure_link(), phy_get_link_state and also the callback are mainly used between the ethernet driver and the phy driver. Between the there we have the MII, the media independent interface, and because of that the ethernet driver doesn't needs to know that media (like copper or fiber) is used. and I like to keep the communication between the drivers the same.
But what I see what we might also need is a api to set the media or used standards of the phy.

Maybe it can be a new field in the phy_link_state struct .

That could be a solution. But that should be part of another PR. I'm a bit more in favor in having separate set and get config api functions for that, so that the existing phy_configure_link() and phy_get_link_state() can stay media independent for the communication with the mac driver.

I am envisioning an enum member field with options of like twisted pair, fiber, or any/unspecified

Do you want that to be part of this PR? I rather wait until we have a phy in tree with support for that change of media(for fist only with setting it via a DT prop) in tree and then open a PR for changing it during runtime.

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Couple of minor nits

@@ -1029,16 +1029,16 @@ static void phy_link_state_change_callback(const struct device *phy_dev,
if (is_up) {
Copy link
Member

Choose a reason for hiding this comment

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

In the first commit drivers: ethernet: phy: rename LINK_*_*BASE_T of this set, the commit description is missing the "why this change" part. Could you add the text in the PR description to the commit message too.

rename  LINK_*_*BASE_T to  LINK_*_*BASE

speed options for ethernet drivers shouldn't end with a _T, implying
that ethernet is only supported via a twisted pair cable.

Signed-off-by: Fin Maaß <[email protected]>
add PHY_LINK_IS_SPEED_10M macro to check if the link speed is 10M.

Signed-off-by: Fin Maaß <[email protected]>
change ETHERNET_LINK_*BASE_T to ETHERNET_LINK_*BASE.

Signed-off-by: Fin Maaß <[email protected]>
mention change of ethernet speed enums.

Signed-off-by: Fin Maaß <[email protected]>
@maass-hamburg maass-hamburg requested review from jukkar and clamattia and removed request for tmon-nordic April 24, 2025 09:20
Copy link
Collaborator

@clamattia clamattia left a comment

Choose a reason for hiding this comment

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

LGTM.
Do we really need both, the LINK_*_*BASE and the ETHERNET_LINK_*BASE enums?
Maybe we could report the half-vs-full info out-of-band and have the same enum for speed in the future?

@maass-hamburg
Copy link
Collaborator Author

@decsny would you be fine, with that, that we add an api for supporting for switching between fiber and copper, when it is needed. In the meantime it could be implemented as a DT property to set that mode at build time. I don't feel good with adding a api right now without implementing it for at least one phy in tree

@decsny decsny dismissed their stale review April 25, 2025 15:09

i will remove the block but do not want to approve the PR, the breaking change seems unnecessary right now

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.

7 participants