Skip to content

usb: device_next: Simple NCM driver for usb-next #72310

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

usb: device_next: Simple NCM driver for usb-next #72310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a802fe2
usb: device_next: Simple NCM driver for usb-next
rgrr Jul 31, 2024
6d39c05
usb: device_next: Simple NCM driver for usb-next
rgrr Jul 31, 2024
0e5407f
usb: device_next: Simple NCM driver for usb-next
rgrr Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion doc/connectivity/usb/device_next/usb_device.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ instance (`n`) and is used as an argument to the :c:func:`usbd_register_class`.
+-----------------------------------+-------------------------+-------------------------+
| USB CDC ECM class | Ethernet device | :samp:`cdc_ecm_{n}` |
+-----------------------------------+-------------------------+-------------------------+
| USB CDC NCM class | Ethernet device | :samp:`cdc_ncm_{n}` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move changes in doc/connectivity/usb/device_next/usb_device.rst to a separate commit, it would also make it easier for you to rebase.

Copy link
Author

Choose a reason for hiding this comment

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

ok... I have created three branches and soon two more PRs will appear here. New code stays in this PR.

Concerning review procedure: who is resolving conversations? You? Me?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One branch (one PR) with N commits is the preferred way.

Copy link
Author

Choose a reason for hiding this comment

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

gosh... what have I done!? Misunderstood Johanns point completely. Will go back.

Copy link
Author

Choose a reason for hiding this comment

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

what commit comments do you expect in this case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If by commit comments you mean the commit messages, then each commit message should describe what the individual commit does. Do not reuse the title for all of the commits, make each of them point to the respective part. For example the commit changing sample should start its commit title with samples: net: zperf:.

Things like "This PR contains the actual driver." do not belong in commit message.

+-----------------------------------+-------------------------+-------------------------+
| USB Mass Storage Class (MSC) | :ref:`usbd_msc_device` | :samp:`msc_{n}` |
+-----------------------------------+-------------------------+-------------------------+
| USB Human Interface Devices (HID) | :ref:`usbd_hid_device` | :samp:`hid_{n}` |
Expand Down Expand Up @@ -74,8 +76,10 @@ configuration ``-DCONF_FILE=usbd_next_prj.conf`` either directly or via

* :zephyr:code-sample:`zperf` To build the sample for the new device support,
set the configuration overlay file
``-DDEXTRA_CONF_FILE=overlay-usbd_next_ecm.conf`` and devicetree overlay file
``-DDEXTRA_CONF_FILE=overlay-usbd_next.conf`` and devicetree overlay file
``-DDTC_OVERLAY_FILE="usbd_next_ecm.overlay`` either directly or via ``west``.
To build with NCM, set the configuration overlay file as above and the devicetree
overlay file to ``-DDTC_OVERLAY_FILE="usbd_next_ncm.overlay``.

How to configure and enable USB device support
**********************************************
Expand Down
16 changes: 16 additions & 0 deletions dts/bindings/ethernet/zephyr,cdc-ncm-ethernet.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Copyright (c) 2024 Hardy Griech
# SPDX-License-Identifier: Apache-2.0

description: USB CDC NCM virtual Ethernet controller

compatible: "zephyr,cdc-ncm-ethernet"

include: ethernet-controller.yaml

properties:
remote-mac-address:
type: string
required: true
description: |
Remote MAC address of the virtual Ethernet connection.
Should not be the same as local-mac-address property.
13 changes: 12 additions & 1 deletion include/zephyr/usb/class/usb_cdc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@
#define ACM_SUBCLASS 0x02
#define ECM_SUBCLASS 0x06
#define EEM_SUBCLASS 0x0c
#define NCM_SUBCLASS 0x0d

/** Communications Class Protocol Codes */
#define AT_CMD_V250_PROTOCOL 0x01
#define EEM_PROTOCOL 0x07
#define ACM_VENDOR_PROTOCOL 0xFF
#define NCM_DATA_PROTOCOL 1

/**
* @brief Data Class Interface Codes
Expand All @@ -50,6 +52,7 @@
#define ACM_FUNC_DESC 0x02
#define UNION_FUNC_DESC 0x06
#define ETHERNET_FUNC_DESC 0x0F
#define ETHERNET_FUNC_DESC_NCM 0x1a

/**
* @brief PSTN Subclass Specific Requests
Expand Down Expand Up @@ -199,7 +202,7 @@ struct cdc_acm_notification {
} __packed;

/** Ethernet Networking Functional Descriptor */
struct cdc_ecm_descriptor {
struct cdc_eth_functional_descriptor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think you should rename it here or at all.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm... but cdc_ecm_descriptor is the wrong name. Will change it back anyway

uint8_t bFunctionLength;
uint8_t bDescriptorType;
uint8_t bDescriptorSubtype;
Expand All @@ -210,4 +213,12 @@ struct cdc_ecm_descriptor {
uint8_t bNumberPowerFilters;
} __packed;

struct cdc_ncm_functional_descriptor {
uint8_t bFunctionLength;
uint8_t bDescriptorType;
uint8_t bDescriptorSubtype;
uint16_t bcdNcmVersion;
uint8_t bmNetworkCapabilities;
} __packed;

#endif /* ZEPHYR_INCLUDE_USB_CLASS_USB_CDC_H_ */
6 changes: 6 additions & 0 deletions samples/net/zperf/overlay-netusb.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
# USB Device Settings
CONFIG_USB_DEVICE_STACK=y

CONFIG_NET_PKT_RX_COUNT=14
CONFIG_NET_PKT_TX_COUNT=14
CONFIG_NET_BUF_RX_COUNT=28
CONFIG_NET_BUF_TX_COUNT=28
CONFIG_NET_BUF_DATA_SIZE=1500

Comment on lines +4 to +9
Copy link
Collaborator

Choose a reason for hiding this comment

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

This configuration overlay is not used if sample is build for the new USB device support. Stray changes?

Copy link
Author

Choose a reason for hiding this comment

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

I have to rethink why I have done this ;-)

# Select USB Configurations
CONFIG_USB_DEVICE_NETWORK_ECM=y
CONFIG_USB_DEVICE_INITIALIZE_AT_BOOT=n
10 changes: 10 additions & 0 deletions samples/net/zperf/overlay-usbd_next.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
CONFIG_USB_DEVICE_STACK_NEXT=y

# next-ncm does not work well with the static-size buffers
CONFIG_NET_PKT_BUF_RX_DATA_POOL_SIZE=10000
CONFIG_NET_PKT_BUF_TX_DATA_POOL_SIZE=10000
CONFIG_NET_BUF_VARIABLE_DATA_SIZE=y

CONFIG_LOG=y
CONFIG_USBD_LOG_LEVEL_WRN=y
CONFIG_UDC_DRIVER_LOG_LEVEL_WRN=y
Comment on lines +1 to +10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move sample changes to a separate commit.
Looks like there should be three commits in your PR, 1) USB NCM implementation + bindings, 2) sample changes, 3) doc update.

Copy link
Author

Choose a reason for hiding this comment

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

done that

5 changes: 0 additions & 5 deletions samples/net/zperf/overlay-usbd_next_ecm.conf
View file
Open in desktop

This file was deleted.

9 changes: 8 additions & 1 deletion samples/net/zperf/sample.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,18 @@ tests:
- native_posix/native/64
sample.net.zperf.device_next_ecm:
harness: net
extra_args: OVERLAY_CONFIG="overlay-usbd_next_ecm.conf"
extra_args: OVERLAY_CONFIG="overlay-usbd_next.conf"
DTC_OVERLAY_FILE="usbd_next_ecm.overlay"
platform_allow: nrf52840dk/nrf52840 frdm_k64f
tags: usb net zperf
depends_on: usb_device
sample.net.zperf.device_next_ncm:
harness: net
extra_args: OVERLAY_CONFIG="overlay-usbd_next.conf"
DTC_OVERLAY_FILE="usbd_next_ncm.overlay"
platform_allow: nrf52840dk/nrf52840 frdm_k64f
tags: usb net zperf
depends_on: usb_device
sample.net.zperf.netusb_eem:
harness: net
extra_args: OVERLAY_CONFIG="overlay-netusb.conf"
Expand Down
12 changes: 12 additions & 0 deletions samples/net/zperf/usbd_next_ncm.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/*
* Copyright (c) 2024 Hardy Griech
*
* SPDX-License-Identifier: Apache-2.0
*/

/ {
cdc_ncm_eth0: cdc_ncm_eth0 {
compatible = "zephyr,cdc-ncm-ethernet";

Check failure on line 9 in samples/net/zperf/usbd_next_ncm.overlay

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

CODE_INDENT 

samples/net/zperf/usbd_next_ncm.overlay:9  code indent should use tabs where possible

Check warning on line 9 in samples/net/zperf/usbd_next_ncm.overlay

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

LEADING_SPACE 

samples/net/zperf/usbd_next_ncm.overlay:9  please, no spaces at the start of a line
remote-mac-address = "00005E005301";

Check failure on line 10 in samples/net/zperf/usbd_next_ncm.overlay

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

CODE_INDENT 

samples/net/zperf/usbd_next_ncm.overlay:10  code indent should use tabs where possible

Check warning on line 10 in samples/net/zperf/usbd_next_ncm.overlay

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

LEADING_SPACE 

samples/net/zperf/usbd_next_ncm.overlay:10  please, no spaces at the start of a line
};

Check warning on line 11 in samples/net/zperf/usbd_next_ncm.overlay

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

LEADING_SPACE 

samples/net/zperf/usbd_next_ncm.overlay:11  please, no spaces at the start of a line
};
Comment on lines +7 to +12
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please indent with tabs, always.

1 change: 1 addition & 0 deletions samples/subsys/usb/common/sample_usbd_init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ static void sample_fix_code_triple(struct usbd_context *uds_ctx,
/* Always use class code information from Interface Descriptors */
if (IS_ENABLED(CONFIG_USBD_CDC_ACM_CLASS) ||
IS_ENABLED(CONFIG_USBD_CDC_ECM_CLASS) ||
IS_ENABLED(CONFIG_USBD_CDC_NCM_CLASS) ||
IS_ENABLED(CONFIG_USBD_AUDIO2_CLASS)) {
/*
* Class with multiple interfaces have an Interface
Expand Down
4 changes: 2 additions & 2 deletions subsys/usb/device/class/netusb/function_ecm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ struct usb_cdc_ecm_config {
struct usb_if_descriptor if0;
struct cdc_header_descriptor if0_header;
struct cdc_union_descriptor if0_union;
struct cdc_ecm_descriptor if0_netfun_ecm;
struct cdc_eth_functional_descriptor if0_netfun_ecm;
struct usb_ep_descriptor if0_int_ep;

struct usb_if_descriptor if1_0;
Expand Down Expand Up @@ -86,7 +86,7 @@ USBD_CLASS_DESCR_DEFINE(primary, 0) struct usb_cdc_ecm_config cdc_ecm_cfg = {
},
/* Ethernet Networking Functional descriptor */
.if0_netfun_ecm = {
.bFunctionLength = sizeof(struct cdc_ecm_descriptor),
.bFunctionLength = sizeof(struct cdc_eth_functional_descriptor),
.bDescriptorType = USB_DESC_CS_INTERFACE,
.bDescriptorSubtype = ETHERNET_FUNC_DESC,
.iMACAddress = 4,
Expand Down
9 changes: 9 additions & 0 deletions subsys/usb/device_next/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ zephyr_library_sources_ifdef(
class/usbd_cdc_ecm.c
)

zephyr_include_directories_ifdef(
CONFIG_USBD_CDC_NCM_CLASS
${ZEPHYR_BASE}/drivers/ethernet
)
zephyr_library_sources_ifdef(
CONFIG_USBD_CDC_NCM_CLASS
class/usbd_cdc_ncm.c
)

zephyr_library_sources_ifdef(
CONFIG_USBD_BT_HCI
class/bt_hci.c
Expand Down
1 change: 1 addition & 0 deletions subsys/usb/device_next/class/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
rsource "Kconfig.loopback"
rsource "Kconfig.cdc_acm"
rsource "Kconfig.cdc_ecm"
rsource "Kconfig.cdc_ncm"
rsource "Kconfig.bt"
rsource "Kconfig.msc"
rsource "Kconfig.uac2"
Expand Down
19 changes: 19 additions & 0 deletions subsys/usb/device_next/class/Kconfig.cdc_ncm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Copyright (c) 2024 Hardy Griech
#
# SPDX-License-Identifier: Apache-2.0

config USBD_CDC_NCM_CLASS
bool "USB CDC NCM implementation [EXPERIMENTAL]"
default y
depends on NET_L2_ETHERNET
depends on DT_HAS_ZEPHYR_CDC_NCM_ETHERNET_ENABLED
help
USB CDC Network Control Model (NCM) implementation"

if USBD_CDC_NCM_CLASS
module = USBD_CDC_NCM
module-str = usbd cdc_ncm
default-count = 1
source "subsys/logging/Kconfig.template.log_config"
rsource "Kconfig.template.instances_count"
endif
4 changes: 2 additions & 2 deletions subsys/usb/device_next/class/usbd_cdc_ecm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ struct usbd_cdc_ecm_desc {
struct usb_if_descriptor if0;
struct cdc_header_descriptor if0_header;
struct cdc_union_descriptor if0_union;
struct cdc_ecm_descriptor if0_ecm;
struct cdc_eth_functional_descriptor if0_ecm;
struct usb_ep_descriptor if0_int_ep;
struct usb_ep_descriptor if0_hs_int_ep;

Expand Down Expand Up @@ -681,7 +681,7 @@ static struct usbd_cdc_ecm_desc cdc_ecm_desc_##n = { \
}, \
\
.if0_ecm = { \
.bFunctionLength = sizeof(struct cdc_ecm_descriptor), \
.bFunctionLength = sizeof(struct cdc_eth_functional_descriptor), \
.bDescriptorType = USB_DESC_CS_INTERFACE, \
.bDescriptorSubtype = ETHERNET_FUNC_DESC, \
.iMACAddress = 0, \
Expand Down
Loading
Loading