drivers: ethernet: use net_if_carrier correct #100809
Conversation
net_if_carrier is to be used independently of the administrative state (start and stop of the ethernet_api). Signed-off-by: Fin Maaß <[email protected]>
maass-hamburg
force-pushed
the
drivers_ethernet_net_if_carrier_fix
branch
from
bcd5442 to
80e8362
Compare
zephyrbot
added
area: Ethernet
platform: NXP S32
zephyrbot
requested review from
ClaCodes,
Dat-NguyenDuy,
congnguyenhuu,
decsny,
jfischer-no,
josuah,
lmajewski,
manuargue,
pdgendt and
tmon-nordic
| LOG_DBG("Start interface %d", net_if_get_by_iface(data->iface)); | ||
|
|
||
| atomic_set_bit(&data->state, CDC_NCM_IFACE_UP); | ||
| net_if_carrier_on(data->iface); |
There was a problem hiding this comment.
This way it is no longer wrong. To really make use of it, net_if_carrier should be changed at the points where a connection is established or disconnected. But that is for later.
Aren't these points simply usbd_cdc_ncm_enable() and usbd_cdc_ncm_disable()? If yes, why not just put the calls there instead of commenting "that is for later".
There was a problem hiding this comment.
Or is it more like usbd_cdc_ncm_shutdown and .update callback in struct usbd_class_api which are bound to functional state (device configured)?
There was a problem hiding this comment.
I now changed it, so that it works with plugging the usb cable out and in. (tested on a renesas ek_ra8d1 with the zperf sample and the provided confs and overlays)
maass-hamburg
force-pushed
the
drivers_ethernet_net_if_carrier_fix
branch
from
80e8362 to
6bc7751
Compare
|
|
||
| atomic_set_bit(&data->state, CDC_NCM_CLASS_SUSPENDED); | ||
|
|
||
| net_if_carrier_off(data->iface); |
There was a problem hiding this comment.
I would argue that suspend is not removing carrier. When suspended the device is still connected to host. It may even be possible for the device to issue Remote Wakeup if device reported that it supports Remote Wakeup and host did allow it (enabled the feature before suspending device).
There was a problem hiding this comment.
But where are we getting the info, that for example the cable is no longer connected?
There was a problem hiding this comment.
What cable do you mean? If "USB cable" then suspend is low power state when the cable is physically connected.
When USB cable is disconnected, all USB classes disable callbacks are called - here it is usbd_cdc_ncm_disable(). When the cable is connected, and device gets configured (so USB functions can operate) then all USB classes enable callbacks are called - here it is usbd_cdc_ncm_enable().
There was a problem hiding this comment.
When USB cable is disconnected, all USB classes disable callbacks are called - here it is
usbd_cdc_ncm_disable(). When the cable is connected, and device gets configured (so USB functions can operate) then all USB classes enable callbacks are called - here it isusbd_cdc_ncm_enable().
No it isn't, at least for the device_next subsys. The disable callbacks (usbd_class_disable) are only called on UDC_EVT_RESET by event_handler_bus_reset.
UDC_EVT_VBUS_REMOVED is not used for that.
There was a problem hiding this comment.
I removed the the net_if_carrier_off() in the suspend therefore.
|
ping @sumitbatra-nxp for the GMAC changes for S32 |
net_if_carrier is to be used independently of the administrative state (start and stop of the ethernet_api). This seem to work. Signed-off-by: Fin Maaß <[email protected]>
maass-hamburg
force-pushed
the
drivers_ethernet_net_if_carrier_fix
branch
from
6bc7751 to
f2aac10
Compare
|
5 participants
net_if_carrier is to be used independently of the
administrative state (start and stop of the ethernet_api).