drivers: ethernet: remove phy related config from eth api #90652
Conversation
remove get configs that are unused by the ethernet mgmt api. Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
maass-hamburg
changed the title
maass-hamburg
changed the title
github-actions
bot
requested review from
asmellby,
clamattia,
danieldegrasse,
decsny,
dkalowsk,
jerome-pouiller,
jhedberg,
jukkar,
kartben,
lmajewski,
manuargue,
Martinhoff-maker,
pdgendt,
rlubos and
ssharks
maass-hamburg
force-pushed
the
remove_phy_functions_from_ethernet
branch
2 times, most recently
from
87a6212 to
31b81a1
Compare
| @@ -501,54 +501,6 @@ static void enc424j600_rx_thread(void *p1, void *p2, void *p3) | |||
| } | |||
| } | |||
|
|
|||
| static int enc424j600_get_config(const struct device *dev, | |||
There was a problem hiding this comment.
It is not clear why you are removing this, app is no longer be able to get the link speed via net_mgmt API?
There was a problem hiding this comment.
from the side of the net_mgmt api ETHERNET_CONFIG_TYPE_LINK ETHERNET_CONFIG_TYPE_DUPLEX and ETHERNET_CONFIG_TYPE_AUTO_NEG were never supported to get only to set. Same for ETHERNET_CONFIG_TYPE_MAC_ADDRESS and ETHERNET_CONFIG_TYPE_PROMISC_MODE.
Take a look in the ethernet_get_config() and ethernet_set_config() function in subsys/net/l2/ethernet/ethernet_mgmt.c. There are some types that can only be set and some that can only be get. For getting the link speed we use the phy_get_link_state(), that was introduced here: #76062.
There was a problem hiding this comment.
There are some types that can only be set and some that can only be get. For getting the link speed we use the
phy_get_link_state(), that was introduced here: #76062.
My next question is that should we allow setting / getting these phy values via net_mgmt? So the Ethernet driver could set the phy values instead of application figuring out the phy and setting these separately.
What about the vendor devs who are working on Ethernet like @decsny, any opinion on this?
There was a problem hiding this comment.
The problem with the current api is:
- it was only meant to be used when autoneg is disabled, so we can only set one mode.
- deactivating autoneg is at least for 1000Base-T problematic, as it requires that autoneg is disabled on both sides, if I remember the ethernet specification correct.
- ETHERNET_CONFIG_TYPE_AUTO_NEG was never used or implemented right, as we currently don't have a phy api to deactivate autoneg, so it was only used in the ethernet driver to know when the values from ETHERNET_CONFIG_TYPE_LINK and ETHERNET_CONFIG_TYPE_DUPLEX are used.
- ETHERNET_CONFIG_TYPE_LINK and ETHERNET_CONFIG_TYPE_DUPLEX are separated, so we have to save the other value in the ethernet driver, also if we change both it has to do a phy_configure twice (with ETHERNET_CONFIG_TYPE_AUTO_NEG 3 times).
- it would lead to duplicated code in the ethernet drivers. The code of drivers/ethernet/dwc_xgmac/eth_dwc_xgmac.c, that I removed, just contains generic stuff, that we would have to add to every ethernet driver and it would make it more difficult to maintain, especially as the ethernet drivers don't need these values, they just have to pass them trough, they already get what they need through the phy callback.
Instead I would be open to something, where the configs of ETHERNET_CONFIG_TYPE_LINK ETHERNET_CONFIG_TYPE_DUPLEX and ETHERNET_CONFIG_TYPE_AUTO_NEG could be set at the same time, so we could bypass the ethernet driver and could directly use phy_configure_link() together with the net_eth_get_phy() in subsys/net/l2/ethernet/ethernet_mgmt.c. But that would not be compatible with the current api.
There was a problem hiding this comment.
I don't think that every app wants to pull in the net management layer, that's why it's optional based on a kconfig.
| @@ -352,30 +352,6 @@ static enum ethernet_hw_caps sy1xx_mac_get_caps(const struct device *dev) | |||
| return supported; | |||
| } | |||
|
|
|||
| static int sy1xx_mac_get_config(const struct device *dev, enum ethernet_config_type type, | |||
| @@ -546,13 +546,6 @@ static int cdc_ecm_set_config(const struct device *dev, | |||
| return -ENOTSUP; | |||
| } | |||
|
|
|||
| static int cdc_ecm_get_config(const struct device *dev, | |||
remove phy related configs from eth config. phy related configs chould go directly into the phy. Most ethernet drivers didn't support the now removed functions yet. Users should instead use `phy_configure_link()` together with the `net_eth_get_phy()` function. Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
Mention ethernet api changes. Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
maass-hamburg
force-pushed
the
remove_phy_functions_from_ethernet
branch
from
31b81a1 to
0eeba09
Compare
|
maass-hamburg
changed the title
phy_configure_link()together with thenet_eth_get_phy()should be used instead. Most drivers already didn't support these removed option either. Only one in tree driver (drivers/ethernet/dwc_xgmac/eth_dwc_xgmac.c) supported these options and it was usingphy_configure_link()internally.